Maulik V Vaghela has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake. ......................................................................
soc/intel/tigerlake: Update fsp params for Jasper Lake.
Update fsp parameters for Jasper Lake SoC. Update fsp parameters for various configuration like Graphics, USB, xDCI, PCI root ports etc.
These are the initial settings for JSL.
BUG=None BRANCH=None TEST=Compilation for jasper lake board is working
Change-Id: Ia8e88f92989fe40d7bd1c28947e005cc0d862fcb Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/soc/intel/tigerlake/fsp_params_jsl.c M src/soc/intel/tigerlake/romstage/fsp_params_jsl.c 2 files changed, 252 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/38461/1
diff --git a/src/soc/intel/tigerlake/fsp_params_jsl.c b/src/soc/intel/tigerlake/fsp_params_jsl.c index 6fb2f9f..3ac1f48 100644 --- a/src/soc/intel/tigerlake/fsp_params_jsl.c +++ b/src/soc/intel/tigerlake/fsp_params_jsl.c @@ -1,7 +1,7 @@ /* * This file is part of the coreboot project. * - * Copyright (C) 2019 Intel Corporation. + * Copyright (C) 2020 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 @@ -13,9 +13,18 @@ * GNU General Public License for more details. */
+#include <console/console.h> #include <fsp/api.h> +#include <fsp/util.h> +#include <fsp/ppi/mp_service_ppi.h> #include <intelblocks/lpss.h> +#include <intelblocks/mp_init.h> +#include <intelblocks/xdci.h> +#include <soc/intel/common/vbt.h> +#include <soc/pci_devs.h> #include <soc/ramstage.h> +#include <soc/soc_chip.h> +#include <string.h>
static const pci_devfn_t serial_io_dev[] = { PCH_DEVFN_I2C0, @@ -32,10 +41,139 @@ PCH_DEVFN_UART2 };
+static void parse_devicetree(FSP_S_CONFIG *params) +{ + const struct soc_intel_tigerlake_config *config; + config = config_of_soc(); + + for (int i = 0; i < CONFIG_SOC_INTEL_I2C_DEV_MAX; i++) + params->SerialIoI2cMode[i] = config->SerialIoI2cMode[i]; + + for (int i = 0; i < CONFIG_SOC_INTEL_COMMON_BLOCK_GSPI_MAX; i++) { + params->SerialIoSpiMode[i] = config->SerialIoGSpiMode[i]; + params->SerialIoSpiCsMode[i] = config->SerialIoGSpiCsMode[i]; + params->SerialIoSpiCsState[i] = config->SerialIoGSpiCsState[i]; + } + + for (int i = 0; i < CONFIG_SOC_INTEL_UART_DEV_MAX; i++) + params->SerialIoUartMode[i] = config->SerialIoUartMode[i]; +} + /* UPD parameters to be initialized before SiliconInit */ void platform_fsp_silicon_init_params_cb(FSPS_UPD *supd) { - /* TODO: Update with UPD override as FSP matures */ + int i; + FSP_S_CONFIG *params = &supd->FspsConfig; + + struct device *dev; + struct soc_intel_tigerlake_config *config; + config = config_of_soc(); + + /* Parse device tree and enable/disable devices */ + parse_devicetree(params); + + /* Load VBT before devicetree-specific config. */ + params->GraphicsConfigPtr = (uintptr_t)vbt_get(); + + /* Check if IGD is present and fill Graphics init param respectively */ + dev = pcidev_path_on_root(SA_DEVFN_IGD); + + if (CONFIG(RUN_FSP_GOP) && dev && dev->enabled) + params->PeiGraphicsPeimInit = 1; + else + params->PeiGraphicsPeimInit = 0; + + /* Set USB OC pin to 0 first */ + for (i = 0; i < ARRAY_SIZE(params->Usb2OverCurrentPin); i++) + params->Usb2OverCurrentPin[i] = 0; + + for (i = 0; i < ARRAY_SIZE(params->Usb3OverCurrentPin); i++) + params->Usb3OverCurrentPin[i] = 0; + + /* Use coreboot MP PPI services if Kconfig is enabled */ + if (CONFIG(USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI)) { + params->CpuMpPpi = (uintptr_t) mp_fill_ppi_services_data(); + params->SkipMpInit = 0; + } else { + params->SkipMpInit = !CONFIG_USE_INTEL_FSP_MP_INIT; + } + + /* Override/Fill Fsp Silicon Param for mainboard */ + mainboard_silicon_init_params(params); + + /* Unlock upper 8 Bytes of RTC RAM */ + params->RtcMemoryLock = 0; + params->PchLockDownBiosLock = 0x1; + params->PchPwrOptEnable = 0x1; + + /* Legacy 8254 timer support */ + params->Enable8254ClockGating = !CONFIG_USE_LEGACY_8254_TIMER; + params->Enable8254ClockGatingOnS3 = 1; + + /* disable Legacy PME */ + memset(params->PcieRpPmSci, 0, sizeof(params->PcieRpPmSci)); + + /* USB configuration */ + for (i = 0; i < ARRAY_SIZE(config->usb2_ports); i++) { + + params->PortUsb20Enable[i] = config->usb2_ports[i].enable; + params->Usb2OverCurrentPin[i] = config->usb2_ports[i].ocpin; + params->Usb2PhyPetxiset[i] = config->usb2_ports[i].pre_emp_bias; + params->Usb2PhyTxiset[i] = config->usb2_ports[i].tx_bias; + params->Usb2PhyPredeemp[i] = config->usb2_ports[i].tx_emp_enable; + params->Usb2PhyPehalfbit[i] = config->usb2_ports[i].pre_emp_bit; + } + + for (i = 0; i < ARRAY_SIZE(config->usb3_ports); i++) { + + params->PortUsb30Enable[i] = config->usb3_ports[i].enable; + params->Usb3OverCurrentPin[i] = config->usb3_ports[i].ocpin; + if (config->usb3_ports[i].tx_de_emp) { + params->Usb3HsioTxDeEmphEnable[i] = 1; + params->Usb3HsioTxDeEmph[i] = config->usb3_ports[i].tx_de_emp; + } + if (config->usb3_ports[i].tx_downscale_amp) { + params->Usb3HsioTxDownscaleAmpEnable[i] = 1; + params->Usb3HsioTxDownscaleAmp[i] = + config->usb3_ports[i].tx_downscale_amp; + } + } + + /* SDCard related configuration */ + dev = pcidev_on_root(PCH_DEV_SLOT_XHCI, 5); + if (!dev) + params->ScsSdCardEnabled = 0; + else { + params->ScsSdCardEnabled = dev->enabled; + params->SdCardPowerEnableActiveHigh = + config->SdCardPowerEnableActiveHigh; + params->SdCardGpioCmdPadTermination = 0x1F; + } + params->Device4Enable = config->Device4Enable; + + /* eMMC configuration */ + dev = pcidev_on_root(PCH_DEV_SLOT_STORAGE, 0); + if (!dev) + params->ScsEmmcEnabled = 0; + else { + params->ScsEmmcEnabled = dev->enabled; + params->ScsEmmcHs400Enabled = config->ScsEmmcHs400Enabled; + } + + /* Enable xDCI controller if enabled in devicetree and allowed */ + dev = pcidev_on_root(PCH_DEV_SLOT_XHCI, 1); + if (!xdci_can_enable()) + dev->enabled = 0; + params->XdciEnable = dev->enabled; + + /* Provide correct UART number for FSP debug logs */ + params->SerialIoDebugUartNumber = CONFIG_UART_FOR_CONSOLE; +} + +/* Mainboard GPIO Configuration */ +__weak void mainboard_silicon_init_params(FSP_S_CONFIG *params) +{ + printk(BIOS_DEBUG, "WEAK: %s/%s called\n", __FILE__, __func__); }
/* Return list of SOC LPSS controllers */ diff --git a/src/soc/intel/tigerlake/romstage/fsp_params_jsl.c b/src/soc/intel/tigerlake/romstage/fsp_params_jsl.c index 810cff4..5cbcda9 100644 --- a/src/soc/intel/tigerlake/romstage/fsp_params_jsl.c +++ b/src/soc/intel/tigerlake/romstage/fsp_params_jsl.c @@ -1,7 +1,7 @@ /* * This file is part of the coreboot project. * - * Copyright (C) 2019 Intel Corp. + * Copyright (C) 2020 Intel Corp. * * 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 @@ -13,10 +13,120 @@ * GNU General Public License for more details. */
+#include <assert.h> +#include <console/console.h> #include <fsp/util.h> +#include <soc/gpio_soc_defs.h> +#include <soc/iomap.h> +#include <soc/pci_devs.h> #include <soc/romstage.h> +#include <soc/soc_chip.h> +#include <string.h> + +static void soc_memory_init_params(FSP_M_CONFIG *m_cfg, + const struct soc_intel_tigerlake_config *config) +{ + unsigned int i; + const struct device *dev = pcidev_path_on_root(SA_DEVFN_IGD); + uint32_t mask = 0; + + if (!dev || !dev->enabled) { + /* + * Skip IGD initialization in FSP if device + * is disable in devicetree.cb. + */ + m_cfg->InternalGfx = 0; + m_cfg->IgdDvmt50PreAlloc = 0; + } else { + m_cfg->InternalGfx = 1; + /* Set IGD stolen size to 60MB. */ + m_cfg->IgdDvmt50PreAlloc = 0xFE; + } + + m_cfg->TsegSize = CONFIG_SMM_TSEG_SIZE; + m_cfg->IedSize = CONFIG_IED_REGION_SIZE; + m_cfg->SaGv = config->SaGv; + m_cfg->RMT = config->RMT; + + /* If Audio Codec is enabled, enable FSP UPD */ + dev = pcidev_path_on_root(PCH_DEVFN_HDA); + if (!dev) + m_cfg->PchHdaEnable = 0; + else + m_cfg->PchHdaEnable = dev->enabled; + for (i = 0; i < ARRAY_SIZE(config->PcieRpEnable); i++) { + if (config->PcieRpEnable[i]) + mask |= (1 << i); + } + + m_cfg->PcieRpEnableMask = mask; + + memcpy(m_cfg->PcieClkSrcUsage, config->PcieClkSrcUsage, + sizeof(config->PcieClkSrcUsage)); + + for (i = 0; i < ARRAY_SIZE(config->PcieClkSrcUsage); i++) { + if (config->PcieClkSrcUsage[i] == 0) + m_cfg->PcieClkSrcUsage[i] = PCIE_CLK_NOTUSED; + } + + memcpy(m_cfg->PcieClkSrcClkReq, config->PcieClkSrcClkReq, + sizeof(config->PcieClkSrcClkReq)); + + m_cfg->PrmrrSize = config->PrmrrSize; + m_cfg->EnableC6Dram = config->enable_c6dram; + + /* Disable BIOS Guard */ + m_cfg->BiosGuard = 0; + + /* Disable Cpu Ratio Override temporary. */ + m_cfg->CpuRatio = 0; + m_cfg->PcdDebugInterfaceFlags = + CONFIG(DRIVERS_UART_8250IO) ? 0x02 : 0x10; + + /* Change VmxEnable UPD value according to ENABLE_VMX Kconfig */ + m_cfg->VmxEnable = CONFIG(ENABLE_VMX); + + m_cfg->SerialIoUartDebugControllerNumber = CONFIG_UART_FOR_CONSOLE; + + /* Audio */ + m_cfg->PchHdaDspEnable = config->PchHdaDspEnable; + m_cfg->PchHdaAudioLinkHdaEnable = config->PchHdaAudioLinkHdaEnable; + m_cfg->PchHdaAudioLinkDmicEnable[0] = config->PchHdaAudioLinkDmicEnable[0]; + m_cfg->PchHdaAudioLinkDmicEnable[1] = config->PchHdaAudioLinkDmicEnable[1]; + for (i = 0; i < ARRAY_SIZE(config->PchHdaAudioLinkSspEnable); i++) { + m_cfg->PchHdaAudioLinkSspEnable[i] = + config->PchHdaAudioLinkSspEnable[i]; + } + for (i = 0; i < ARRAY_SIZE(config->PchHdaAudioLinkSndwEnable); i++) { + m_cfg->PchHdaAudioLinkSndwEnable[i] = + config->PchHdaAudioLinkSndwEnable[i]; + } +}
void platform_fsp_memory_init_params_cb(FSPM_UPD *mupd, uint32_t version) { - /* TODO: Update with UPD override as FSP matures */ + const struct soc_intel_tigerlake_config *config; + FSP_M_CONFIG *m_cfg = &mupd->FspmConfig; + config = config_of_soc(); + + soc_memory_init_params(m_cfg, config); + + /* Enable SMBus controller based on config */ + m_cfg->SmbusEnable = config->SmbusEnable; + + /* Set debug probe type */ + m_cfg->PlatformDebugConsent = config->DebugConsent; + + /* Vt-D config */ + m_cfg->VtdDisable = 0; + + /* MbHob */ + m_cfg->SkipMbpHob = 0; + + mainboard_memory_init_params(mupd); +} + +__weak void mainboard_memory_init_params(FSPM_UPD *mupd) +{ + printk(BIOS_DEBUG, "WEAK: %s/%s called\n", __FILE__, __func__); }
Hello Patrick Rudolph, Karthikeyan Ramasubramanian, Subrata Banik, Aamir Bohra, Rizwan Qureshi, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38461
to look at the new patch set (#2).
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake. ......................................................................
soc/intel/tigerlake: Update fsp params for Jasper Lake.
Update fsp parameters for Jasper Lake SoC. Update fsp parameters for various configuration like Graphics, USB, xDCI, PCI root ports etc.
These are the initial settings for JSL.
BUG=None BRANCH=None TEST=Compilation for jasper lake board is working
Change-Id: Ia8e88f92989fe40d7bd1c28947e005cc0d862fcb Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/soc/intel/tigerlake/fsp_params_jsl.c M src/soc/intel/tigerlake/romstage/fsp_params_jsl.c 2 files changed, 239 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/38461/2
Hello Patrick Rudolph, Karthikeyan Ramasubramanian, Subrata Banik, Aamir Bohra, Rizwan Qureshi, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38461
to look at the new patch set (#3).
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake. ......................................................................
soc/intel/tigerlake: Update fsp params for Jasper Lake.
Update fsp parameters for Jasper Lake SoC. Update fsp parameters for various configuration like Graphics, USB, xDCI, PCI root ports etc.
These are the initial settings for JSL.
BUG=None BRANCH=None TEST=Compilation for jasper lake board is working
Change-Id: Ia8e88f92989fe40d7bd1c28947e005cc0d862fcb Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/soc/intel/tigerlake/fsp_params_jsl.c M src/soc/intel/tigerlake/romstage/fsp_params_jsl.c 2 files changed, 237 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/38461/3
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake. ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38461/3/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/3/src/soc/intel/tigerlake/fsp... PS3, Line 86: /* Set USB OC pin to 0 first */ : for (i = 0; i < ARRAY_SIZE(params->Usb2OverCurrentPin); i++) : params->Usb2OverCurrentPin[i] = 0; : : for (i = 0; i < ARRAY_SIZE(params->Usb3OverCurrentPin); i++) : params->Usb3OverCurrentPin[i] = 0; : not needed?
https://review.coreboot.org/c/coreboot/+/38461/3/src/soc/intel/tigerlake/fsp... PS3, Line 101: /* Override/Fill Fsp Silicon Param for mainboard */ : mainboard_silicon_init_params(params); can we move this to end after local assignments
https://review.coreboot.org/c/coreboot/+/38461/3/src/soc/intel/tigerlake/rom... File src/soc/intel/tigerlake/romstage/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/3/src/soc/intel/tigerlake/rom... PS3, Line 99: soc_memory_init_params(m_cfg, config); move at end after local assignments?
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake. ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38461/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38461/3//COMMIT_MSG@7 PS3, Line 7: soc/intel/tigerlake: Update fsp params for Jasper Lake. Please remove the dot/period at the end.
https://review.coreboot.org/c/coreboot/+/38461/3//COMMIT_MSG@10 PS3, Line 10: Graphics graphics
https://review.coreboot.org/c/coreboot/+/38461/3//COMMIT_MSG@9 PS3, Line 9: Update fsp parameters for Jasper Lake SoC. : Update fsp parameters for various configuration like Graphics, USB, xDCI, : PCI root ports etc. Please format this as a list/enumeration.
Hello Patrick Rudolph, Karthikeyan Ramasubramanian, Subrata Banik, Ronak Kanabar, Aamir Bohra, Wonkyu Kim, Rizwan Qureshi, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38461
to look at the new patch set (#4).
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
soc/intel/tigerlake: Update fsp params for Jasper Lake
Update fsp parameters for various configurations like: - graphics - usb - pci root ports - sd card - emmc - basic uart configuration etc.
These are the initial settings for JSL
BUG=None BRANCH=None TEST=Compilation for jasper lake board is working
Change-Id: Ia8e88f92989fe40d7bd1c28947e005cc0d862fcb Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/soc/intel/tigerlake/fsp_params_jsl.c M src/soc/intel/tigerlake/romstage/fsp_params_jsl.c 2 files changed, 230 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/38461/4
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38461/4/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/4/src/soc/intel/tigerlake/fsp... PS4, Line 159: trailing whitespace
Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 4:
(6 comments)
https://review.coreboot.org/c/coreboot/+/38461/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38461/3//COMMIT_MSG@7 PS3, Line 7: soc/intel/tigerlake: Update fsp params for Jasper Lake.
Please remove the dot/period at the end.
Done
https://review.coreboot.org/c/coreboot/+/38461/3//COMMIT_MSG@10 PS3, Line 10: Graphics
graphics
Done
https://review.coreboot.org/c/coreboot/+/38461/3//COMMIT_MSG@9 PS3, Line 9: Update fsp parameters for Jasper Lake SoC. : Update fsp parameters for various configuration like Graphics, USB, xDCI, : PCI root ports etc.
Please format this as a list/enumeration.
Done
https://review.coreboot.org/c/coreboot/+/38461/3/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/3/src/soc/intel/tigerlake/fsp... PS3, Line 86: /* Set USB OC pin to 0 first */ : for (i = 0; i < ARRAY_SIZE(params->Usb2OverCurrentPin); i++) : params->Usb2OverCurrentPin[i] = 0; : : for (i = 0; i < ARRAY_SIZE(params->Usb3OverCurrentPin); i++) : params->Usb3OverCurrentPin[i] = 0; :
not needed?
Done
https://review.coreboot.org/c/coreboot/+/38461/3/src/soc/intel/tigerlake/fsp... PS3, Line 101: /* Override/Fill Fsp Silicon Param for mainboard */ : mainboard_silicon_init_params(params);
can we move this to end after local assignments
Done
https://review.coreboot.org/c/coreboot/+/38461/3/src/soc/intel/tigerlake/rom... File src/soc/intel/tigerlake/romstage/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/3/src/soc/intel/tigerlake/rom... PS3, Line 99: soc_memory_init_params(m_cfg, config);
move at end after local assignments?
Done
Hello Patrick Rudolph, Karthikeyan Ramasubramanian, Subrata Banik, Ronak Kanabar, Aamir Bohra, Wonkyu Kim, Rizwan Qureshi, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38461
to look at the new patch set (#5).
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
soc/intel/tigerlake: Update fsp params for Jasper Lake
Update fsp parameters for various configurations like: - graphics - usb - pci root ports - sd card - emmc - basic uart configuration etc.
These are the initial settings for JSL
BUG=None BRANCH=None TEST=Compilation for jasper lake board is working
Change-Id: Ia8e88f92989fe40d7bd1c28947e005cc0d862fcb Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/soc/intel/tigerlake/fsp_params_jsl.c M src/soc/intel/tigerlake/romstage/fsp_params_jsl.c 2 files changed, 230 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/38461/5
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 5:
(5 comments)
https://review.coreboot.org/c/coreboot/+/38461/5/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/5/src/soc/intel/tigerlake/fsp... PS5, Line 4: 2020 2019-2020
https://review.coreboot.org/c/coreboot/+/38461/5/src/soc/intel/tigerlake/rom... File src/soc/intel/tigerlake/romstage/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/5/src/soc/intel/tigerlake/rom... PS5, Line 16: #include <assert.h> needed?
https://review.coreboot.org/c/coreboot/+/38461/5/src/soc/intel/tigerlake/rom... PS5, Line 19: #include <soc/gpio_soc_defs.h> : #include <soc/iomap.h> needed?
https://review.coreboot.org/c/coreboot/+/38461/5/src/soc/intel/tigerlake/rom... PS5, Line 57: for (i = 0; i < ARRAY_SIZE(config->PcieRpEnable); i++) { new line /* Pcie Root port configuration */
https://review.coreboot.org/c/coreboot/+/38461/5/src/soc/intel/tigerlake/rom... PS5, Line 108: /* MbHob */ : m_cfg->SkipMbpHob = 0; not needed.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38461/5/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/5/src/soc/intel/tigerlake/fsp... PS5, Line 72: enable/disable devices Nit: I am not seeing any enable/disable device actions besides parsing the devicetree. May be the comment needs some update?
https://review.coreboot.org/c/coreboot/+/38461/5/src/soc/intel/tigerlake/fsp... PS5, Line 86: MP PPI Just curious: What is MP PPI?
https://review.coreboot.org/c/coreboot/+/38461/5/src/soc/intel/tigerlake/fsp... PS5, Line 132: if (!dev) Nit: Use "{" because else block has braces. Just to improve the readability. Same comment for line 143.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 5:
(1 comment)
Also will the FSP params for CNVI, HECI be configured in a follow-up CL?
https://review.coreboot.org/c/coreboot/+/38461/5/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/5/src/soc/intel/tigerlake/fsp... PS5, Line 143: PCH_DEV_SLOT_STORAGE Where is this defined? In an earlier patch, BDF for eMMC got removed from tigerlake definitions.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38461/5/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/5/src/soc/intel/tigerlake/fsp... PS5, Line 143: PCH_DEV_SLOT_STORAGE
Where is this defined? In an earlier patch, BDF for eMMC got removed from tigerlake definitions.
Referring to this CL here: https://review.coreboot.org/c/coreboot/+/38341/12/src/soc/intel/tigerlake/in...
Hello Patrick Rudolph, Karthikeyan Ramasubramanian, Subrata Banik, Aamir Bohra, Ronak Kanabar, Wonkyu Kim, Rizwan Qureshi, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38461
to look at the new patch set (#6).
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
soc/intel/tigerlake: Update fsp params for Jasper Lake
Update fsp parameters for various configurations like: - graphics - usb - pci root ports - sd card - emmc - basic uart configuration etc.
These are the initial settings for JSL
Also added macro definition for emmc devices to prevent compilation issues.
BUG=None BRANCH=None TEST=Compilation for jasper lake board is working
Change-Id: Ia8e88f92989fe40d7bd1c28947e005cc0d862fcb Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/soc/intel/tigerlake/fsp_params_jsl.c M src/soc/intel/tigerlake/include/soc/pci_devs.h M src/soc/intel/tigerlake/romstage/fsp_params_jsl.c 3 files changed, 231 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/38461/6
Hello Patrick Rudolph, Karthikeyan Ramasubramanian, Subrata Banik, Aamir Bohra, Ronak Kanabar, Wonkyu Kim, Rizwan Qureshi, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38461
to look at the new patch set (#7).
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
soc/intel/tigerlake: Update fsp params for Jasper Lake
Update fsp parameters for various configurations like: - graphics - usb - pci root ports - sd card - emmc - basic uart configuration etc.
These are the initial settings for JSL
BUG=None BRANCH=None TEST=Compilation for jasper lake board is working
Change-Id: Ia8e88f92989fe40d7bd1c28947e005cc0d862fcb Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/soc/intel/tigerlake/fsp_params_jsl.c M src/soc/intel/tigerlake/include/soc/pci_devs.h M src/soc/intel/tigerlake/romstage/fsp_params_jsl.c 3 files changed, 230 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/38461/7
Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 7:
(9 comments)
https://review.coreboot.org/c/coreboot/+/38461/5/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/5/src/soc/intel/tigerlake/fsp... PS5, Line 4: 2020
2019-2020
Done
https://review.coreboot.org/c/coreboot/+/38461/5/src/soc/intel/tigerlake/fsp... PS5, Line 72: enable/disable devices
Nit: I am not seeing any enable/disable device actions besides parsing the devicetree. […]
Done
https://review.coreboot.org/c/coreboot/+/38461/5/src/soc/intel/tigerlake/fsp... PS5, Line 86: MP PPI
Just curious: What is MP PPI?
MP PPI is the coreboot implementation of initializing various CPU specific feature. We publish PPI to FSP and FSP can call coreboot PPI to run feature initialization on all APs. You can find more details here: https://review.coreboot.org/c/coreboot/+/31841
https://review.coreboot.org/c/coreboot/+/38461/5/src/soc/intel/tigerlake/fsp... PS5, Line 132: if (!dev)
Nit: Use "{" because else block has braces. Just to improve the readability. […]
Done
https://review.coreboot.org/c/coreboot/+/38461/5/src/soc/intel/tigerlake/fsp... PS5, Line 143: PCH_DEV_SLOT_STORAGE
Referring to this CL here: https://review.coreboot. […]
Done
https://review.coreboot.org/c/coreboot/+/38461/5/src/soc/intel/tigerlake/rom... File src/soc/intel/tigerlake/romstage/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/5/src/soc/intel/tigerlake/rom... PS5, Line 16: #include <assert.h>
needed?
Done
https://review.coreboot.org/c/coreboot/+/38461/5/src/soc/intel/tigerlake/rom... PS5, Line 19: #include <soc/gpio_soc_defs.h> : #include <soc/iomap.h>
needed?
Done
https://review.coreboot.org/c/coreboot/+/38461/5/src/soc/intel/tigerlake/rom... PS5, Line 57: for (i = 0; i < ARRAY_SIZE(config->PcieRpEnable); i++) {
new line […]
Done
https://review.coreboot.org/c/coreboot/+/38461/5/src/soc/intel/tigerlake/rom... PS5, Line 108: /* MbHob */ : m_cfg->SkipMbpHob = 0;
not needed.
Done
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38461/7/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/7/src/soc/intel/tigerlake/fsp... PS7, Line 132: if (!dev){ space required before the open brace '{'
https://review.coreboot.org/c/coreboot/+/38461/7/src/soc/intel/tigerlake/fsp... PS7, Line 144: if (!dev){ space required before the open brace '{'
Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38461/5/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/5/src/soc/intel/tigerlake/fsp... PS5, Line 143: PCH_DEV_SLOT_STORAGE
Done
I have added definition for BDF for eMMC since we'll need it for JSL
Hello Patrick Rudolph, Karthikeyan Ramasubramanian, Subrata Banik, Aamir Bohra, Ronak Kanabar, Wonkyu Kim, Rizwan Qureshi, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38461
to look at the new patch set (#8).
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
soc/intel/tigerlake: Update fsp params for Jasper Lake
Update fsp parameters for various configurations like: - graphics - usb - pci root ports - sd card - emmc - basic uart configuration etc.
These are the initial settings for JSL
BUG=None BRANCH=None TEST=Compilation for jasper lake board is working
Change-Id: Ia8e88f92989fe40d7bd1c28947e005cc0d862fcb Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/soc/intel/tigerlake/fsp_params_jsl.c M src/soc/intel/tigerlake/include/soc/pci_devs.h M src/soc/intel/tigerlake/romstage/fsp_params_jsl.c 3 files changed, 230 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/38461/8
Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 8:
Patch Set 5:
(1 comment)
Also will the FSP params for CNVI, HECI be configured in a follow-up CL?
Yes, Karthik. This is just a base config patch.
Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 8:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38461/7/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/7/src/soc/intel/tigerlake/fsp... PS7, Line 132: if (!dev){
space required before the open brace '{'
Done
https://review.coreboot.org/c/coreboot/+/38461/7/src/soc/intel/tigerlake/fsp... PS7, Line 144: if (!dev){
space required before the open brace '{'
Done
Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38461/5/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/5/src/soc/intel/tigerlake/fsp... PS5, Line 86: MP PPI
MP PPI is the coreboot implementation of initializing various CPU specific feature. […]
Done
Hello Patrick Rudolph, Karthikeyan Ramasubramanian, Subrata Banik, Aamir Bohra, Ronak Kanabar, Wonkyu Kim, Rizwan Qureshi, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38461
to look at the new patch set (#9).
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
soc/intel/tigerlake: Update fsp params for Jasper Lake
Update fsp parameters for various configurations like: - graphics - usb - pci root ports - sd card - emmc - audio - basic uart configuration etc.
These are the initial settings for JSL
BUG=None BRANCH=None TEST=Compilation for jasper lake board is working
Change-Id: Ia8e88f92989fe40d7bd1c28947e005cc0d862fcb Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/soc/intel/tigerlake/fsp_params_jsl.c M src/soc/intel/tigerlake/include/soc/pci_devs.h M src/soc/intel/tigerlake/romstage/fsp_params_jsl.c 3 files changed, 245 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/38461/9
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 10: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 10:
(12 comments)
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/fs... File src/soc/intel/tigerlake/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/fs... PS10, Line 50: SerialIoI2cMode Just curious: Can we not use memcpy for the params here?
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/fs... PS10, Line 70: config = config_of_soc(); This can potentially go onto the previous line?
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/fs... PS10, Line 131: pcidev_on_root(PCH_DEV_SLOT_XHCI, 5); Why not use pcidev_path_on_root(PCH_DEVFN_...);
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/fs... PS10, Line 138: params->SdCardGpioCmdPadTermination = 0x1F; What does this mean? Shouldn't this be mainboard controlled just like the active high config above?
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/fs... PS10, Line 143: pcidev_on_root(PCH_DEV_SLOT_STORAGE, 0); same comment as above.
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/fs... PS10, Line 152: pcidev_on_root(PCH_DEV_SLOT_XHCI, 1); same comment as above.
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/pci_devs.h:
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/in... PS10, Line 125: #define PCH_DEV_SLOT_STORAGE 0x1a : #define PCH_DEVFN_EMMC _PCH_DEVFN(STORAGE, 0) : #define PCH_DEV_EMMC _PCH_DEV(STORAGE, 0) Is this true for both TGL and JSL?
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/ro... File src/soc/intel/tigerlake/romstage/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/ro... PS10, Line 80: temporary why? and for how long?
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/ro... PS10, Line 83: 0x02 fsp_params_tgl.c uses some enums here. Can we do the same for JSL?
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/ro... PS10, Line 96: for (i = 0; i < ARRAY_SIZE(config->PchHdaAudioLinkSspEnable); i++) { : m_cfg->PchHdaAudioLinkSspEnable[i] = : config->PchHdaAudioLinkSspEnable[i]; : } Would memcpy work?
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/ro... PS10, Line 101: for (i = 0; i < ARRAY_SIZE(config->PchHdaAudioLinkSndwEnable); i++) { : m_cfg->PchHdaAudioLinkSndwEnable[i] = : config->PchHdaAudioLinkSndwEnable[i]; : } Would memcpy work?
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/ro... PS10, Line 113: /* Enable SMBus controller based on config */ : m_cfg->SmbusEnable = config->SmbusEnable; : : /* Set debug probe type */ : m_cfg->PlatformDebugConsent = config->DebugConsent; : : /* Vt-D config */ : m_cfg->VtdDisable = 0; Why are these not done as part of soc_memory_init_params?
Aamir Bohra has uploaded a new patch set (#11) to the change originally created by Maulik V Vaghela. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
soc/intel/tigerlake: Update fsp params for Jasper Lake
Update fsp parameters for various configurations like: - graphics - usb - pci root ports - sd card - emmc - audio - basic uart configuration etc.
These are the initial settings for JSL
BUG=None BRANCH=None TEST=Compilation for jasper lake board is working
Change-Id: Ia8e88f92989fe40d7bd1c28947e005cc0d862fcb Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/soc/intel/tigerlake/chip.h M src/soc/intel/tigerlake/fsp_params_jsl.c M src/soc/intel/tigerlake/include/soc/pci_devs.h M src/soc/intel/tigerlake/romstage/fsp_params_jsl.c M src/soc/intel/tigerlake/romstage/fsp_params_tgl.c 5 files changed, 258 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/38461/11
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 11:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38461/11/src/soc/intel/tigerlake/ch... File src/soc/intel/tigerlake/chip.h:
https://review.coreboot.org/c/coreboot/+/38461/11/src/soc/intel/tigerlake/ch... PS11, Line 214: }debuginterfaceflag; space required after that close brace '}'
https://review.coreboot.org/c/coreboot/+/38461/11/src/soc/intel/tigerlake/fs... File src/soc/intel/tigerlake/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/11/src/soc/intel/tigerlake/fs... PS11, Line 136: if (!dev) { braces {} are not necessary for any arm of this statement
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 11:
(12 comments)
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/fs... File src/soc/intel/tigerlake/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/fs... PS10, Line 50: SerialIoI2cMode
Just curious: Can we not use memcpy for the params here?
Done
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/fs... PS10, Line 70: config = config_of_soc();
This can potentially go onto the previous line?
Done
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/fs... PS10, Line 131: pcidev_on_root(PCH_DEV_SLOT_XHCI, 5);
Why not use pcidev_path_on_root(PCH_DEVFN_... […]
Done
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/fs... PS10, Line 138: params->SdCardGpioCmdPadTermination = 0x1F;
What does this mean? Shouldn't this be mainboard controlled just like the active high config above?
Set the PU/PD configuration for SD card CMD GPIO PAD, agree needs to be moved to main board gpio configuration and not through FSP. Setting it to 0(dsc default) would allow FSP to skip it.
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/fs... PS10, Line 143: pcidev_on_root(PCH_DEV_SLOT_STORAGE, 0);
same comment as above.
Done
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/fs... PS10, Line 152: pcidev_on_root(PCH_DEV_SLOT_XHCI, 1);
same comment as above.
Done
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/pci_devs.h:
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/in... PS10, Line 125: #define PCH_DEV_SLOT_STORAGE 0x1a : #define PCH_DEVFN_EMMC _PCH_DEVFN(STORAGE, 0) : #define PCH_DEV_EMMC _PCH_DEV(STORAGE, 0)
Is this true for both TGL and JSL?
No, guarded for JSL now.
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/ro... File src/soc/intel/tigerlake/romstage/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/ro... PS10, Line 80: temporary
why? and for how long?
This was a wrong comment, we are not setting the override UPD(CpuRatioOverride) here, instead setting flex ratio for HFM
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/ro... PS10, Line 83: 0x02
fsp_params_tgl.c uses some enums here. […]
Done
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/ro... PS10, Line 96: for (i = 0; i < ARRAY_SIZE(config->PchHdaAudioLinkSspEnable); i++) { : m_cfg->PchHdaAudioLinkSspEnable[i] = : config->PchHdaAudioLinkSspEnable[i]; : }
Would memcpy work?
Done
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/ro... PS10, Line 101: for (i = 0; i < ARRAY_SIZE(config->PchHdaAudioLinkSndwEnable); i++) { : m_cfg->PchHdaAudioLinkSndwEnable[i] = : config->PchHdaAudioLinkSndwEnable[i]; : }
Would memcpy work?
Done
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/ro... PS10, Line 113: /* Enable SMBus controller based on config */ : m_cfg->SmbusEnable = config->SmbusEnable; : : /* Set debug probe type */ : m_cfg->PlatformDebugConsent = config->DebugConsent; : : /* Vt-D config */ : m_cfg->VtdDisable = 0;
Why are these not done as part of soc_memory_init_params?
Done
Aamir Bohra has uploaded a new patch set (#12) to the change originally created by Maulik V Vaghela. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
soc/intel/tigerlake: Update fsp params for Jasper Lake
Update fsp parameters for various configurations like: - graphics - usb - pci root ports - sd card - emmc - audio - basic uart configuration etc.
These are the initial settings for JSL
BUG=None BRANCH=None TEST=Compilation for jasper lake board is working
Change-Id: Ia8e88f92989fe40d7bd1c28947e005cc0d862fcb Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/soc/intel/tigerlake/chip.h M src/soc/intel/tigerlake/fsp_params_jsl.c M src/soc/intel/tigerlake/include/soc/pci_devs.h M src/soc/intel/tigerlake/romstage/fsp_params_jsl.c M src/soc/intel/tigerlake/romstage/fsp_params_tgl.c 5 files changed, 258 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/38461/12
Aamir Bohra has uploaded a new patch set (#14) to the change originally created by Maulik V Vaghela. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
soc/intel/tigerlake: Update fsp params for Jasper Lake
Update fsp parameters for various configurations like: - graphics - usb - pci root ports - sd card - emmc - audio - basic uart configuration etc.
These are the initial settings for JSL
BUG=None BRANCH=None TEST=Compilation for jasper lake board is working
Change-Id: Ia8e88f92989fe40d7bd1c28947e005cc0d862fcb Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/soc/intel/tigerlake/chip.h M src/soc/intel/tigerlake/fsp_params_jsl.c M src/soc/intel/tigerlake/include/soc/pci_devs.h M src/soc/intel/tigerlake/romstage/fsp_params_jsl.c M src/soc/intel/tigerlake/romstage/fsp_params_tgl.c 5 files changed, 258 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/38461/14
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 16:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/fs... File src/soc/intel/tigerlake/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/fs... PS16, Line 19: #include <fsp/ppi/mp_service_ppi.h> Move it one line above so that it is consistent with the alphabetical order of include.
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/fs... PS16, Line 47: config = config_of_soc(); Nit: declare and initialize in the previous line itself.
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/fs... PS16, Line 65: sizeof(config->SerialIoUartMode)); Just curious: FSP UPD seems to support 7 I2C/GSPI/UART ports whereas in JSL there are 6,3 & 3 ports respectively. Are the UPDs for remaining ports in some reset/initialized state?
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/ch... File src/soc/intel/tigerlake/chip.h:
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/ch... PS16, Line 208: enum { : DEBUG_INTERFACE_RAM = 0x1, : DEBUG_INTERFACE_UART = 0x2, : DEBUG_INTERFACE_USB3 = 0x4, : DEBUG_INTERFACE_SERIAL_IO = 0x8, : DEBUG_INTERFACE_TRACEHUB = 0x10 : } debug_interface_flag; As per the FSP UPD, Bit 2 is not used. But here it is used. Please find the below comment in the Fspmupd.h
Debug Interfaces. BIT0-RAM, BIT1-UART, BIT3-USB3, BIT4-Serial IO, BIT5-TraceHub, BIT2 - Not used.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 16:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/ro... File src/soc/intel/tigerlake/romstage/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/ro... PS16, Line 48: /* If Audio Codec is enabled, enable FSP UPD */ : dev = pcidev_path_on_root(PCH_DEVFN_HDA); : if (!dev) : m_cfg->PchHdaEnable = 0; : else : m_cfg->PchHdaEnable = dev->enabled; Can this be moved with the other Audio related configuration below for better grouping?
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/ro... PS16, Line 67: config->PcieClkSrcUsage[i] == 0 I am not sure if it cannot take the value of 0. As per the comments in tigerlake's chip.h, it can take 0-23: PCH rootport, 0x70: LAN, 0x80: unspecified but in use, 0xFF: not used.
Why can't we configure 0xFF explicitly?
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/ro... PS16, Line 103: m_cfg->PchHdaAudioLinkDmicEnable[0] = config->PchHdaAudioLinkDmicEnable[0]; : m_cfg->PchHdaAudioLinkDmicEnable[1] = config->PchHdaAudioLinkDmicEnable[1]; Use memcpy
Aamir Bohra has uploaded a new patch set (#17) to the change originally created by Maulik V Vaghela. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
soc/intel/tigerlake: Update fsp params for Jasper Lake
Update fsp parameters for various configurations like: - graphics - usb - pci root ports - sd card - emmc - audio - basic uart configuration etc.
These are the initial settings for JSL
BUG=None BRANCH=None TEST=Compilation for jasper lake board is working
Change-Id: Ia8e88f92989fe40d7bd1c28947e005cc0d862fcb Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/soc/intel/tigerlake/chip.h M src/soc/intel/tigerlake/fsp_params_jsl.c M src/soc/intel/tigerlake/include/soc/pci_devs.h M src/soc/intel/tigerlake/romstage/fsp_params_jsl.c M src/soc/intel/tigerlake/romstage/fsp_params_tgl.c 5 files changed, 257 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/38461/17
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 17:
(6 comments)
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/fs... File src/soc/intel/tigerlake/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/fs... PS16, Line 19: #include <fsp/ppi/mp_service_ppi.h>
Move it one line above so that it is consistent with the alphabetical order of include.
Done
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/fs... PS16, Line 47: config = config_of_soc();
Nit: declare and initialize in the previous line itself.
Done
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/fs... PS16, Line 65: sizeof(config->SerialIoUartMode));
Just curious: FSP UPD seems to support 7 I2C/GSPI/UART ports whereas in JSL there are 6,3 & 3 ports […]
They are more as reserved fields, the controller number supported is retrieved in FSP and is specific to platform, for JSL it returns 6,3 & 3 for i2C, UART and GSPI respectively.
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/ro... File src/soc/intel/tigerlake/romstage/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/ro... PS16, Line 48: /* If Audio Codec is enabled, enable FSP UPD */ : dev = pcidev_path_on_root(PCH_DEVFN_HDA); : if (!dev) : m_cfg->PchHdaEnable = 0; : else : m_cfg->PchHdaEnable = dev->enabled;
Can this be moved with the other Audio related configuration below for better grouping?
Done
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/ro... PS16, Line 67: config->PcieClkSrcUsage[i] == 0
I am not sure if it cannot take the value of 0. As per the comments in tigerlake's chip. […]
if config does not get set explicity through the devicetree(remains 0), this would ensure the Clkrsrc ic configuerd as not used.
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/ro... PS16, Line 103: m_cfg->PchHdaAudioLinkDmicEnable[0] = config->PchHdaAudioLinkDmicEnable[0]; : m_cfg->PchHdaAudioLinkDmicEnable[1] = config->PchHdaAudioLinkDmicEnable[1];
Use memcpy
Done
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/ch... File src/soc/intel/tigerlake/chip.h:
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/ch... PS16, Line 208: enum { : DEBUG_INTERFACE_RAM = 0x1, : DEBUG_INTERFACE_UART = 0x2, : DEBUG_INTERFACE_USB3 = 0x4, : DEBUG_INTERFACE_SERIAL_IO = 0x8, : DEBUG_INTERFACE_TRACEHUB = 0x10 : } debug_interface_flag;
As per the FSP UPD, Bit 2 is not used. But here it is used. […]
the description seems to be incorrect, I see, it is getting used for IO UART read/write in the fsp.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 18:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/ch... File src/soc/intel/tigerlake/chip.h:
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/ch... PS16, Line 208: enum { : DEBUG_INTERFACE_RAM = 0x1, : DEBUG_INTERFACE_UART = 0x2, : DEBUG_INTERFACE_USB3 = 0x4, : DEBUG_INTERFACE_SERIAL_IO = 0x8, : DEBUG_INTERFACE_TRACEHUB = 0x10 : } debug_interface_flag;
the description seems to be incorrect, I see, it is getting used for IO UART read/write in the fsp.
The description does not say about UART. It is saying about the bits for USB3, SERIAL_IO & TRACEHUB i.e DEBUG_INTERFACE_USB3 should be 0x8 and not 0x4, DEBUG_INTERFACE_SERIAL_IO should be 0x10 and DEBUG_INTERFACE_TRACEHUB should be 0x20.
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/ro... File src/soc/intel/tigerlake/romstage/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/ro... PS16, Line 67: config->PcieClkSrcUsage[i] == 0
if config does not get set explicity through the devicetree(remains 0), this would ensure the Clkrs […]
I am still confused. As per tigerlake's chip.h, 0 corresponds to PCH Root port. But the code here marks it as unused. This seems conflicting information.
If PcieClkSrcUsage cannot be set to 0, then can you please update the comment in tigerlake's chip.h
Aamir Bohra has uploaded a new patch set (#19) to the change originally created by Maulik V Vaghela. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
soc/intel/tigerlake: Update fsp params for Jasper Lake
Update fsp parameters for various configurations like: - graphics - usb - pci root ports - sd card - emmc - audio - basic uart configuration etc.
These are the initial settings for JSL This patch also corrects the debug_interface_flag definitions.
BUG=None BRANCH=None TEST=Compilation for jasper lake board is working
Change-Id: Ia8e88f92989fe40d7bd1c28947e005cc0d862fcb Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/soc/intel/tigerlake/chip.h M src/soc/intel/tigerlake/fsp_params_jsl.c M src/soc/intel/tigerlake/include/soc/pci_devs.h M src/soc/intel/tigerlake/romstage/fsp_params_jsl.c M src/soc/intel/tigerlake/romstage/fsp_params_tgl.c 5 files changed, 252 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/38461/19
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 20:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/ch... File src/soc/intel/tigerlake/chip.h:
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/ch... PS16, Line 208: enum { : DEBUG_INTERFACE_RAM = 0x1, : DEBUG_INTERFACE_UART = 0x2, : DEBUG_INTERFACE_USB3 = 0x4, : DEBUG_INTERFACE_SERIAL_IO = 0x8, : DEBUG_INTERFACE_TRACEHUB = 0x10 : } debug_interface_flag;
The description does not say about UART. […]
Sorry , my bad, Updated the flags, the dsc description now holds valid too.
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/ro... File src/soc/intel/tigerlake/romstage/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/ro... PS16, Line 67: config->PcieClkSrcUsage[i] == 0
I am still confused. As per tigerlake's chip.h, 0 corresponds to PCH Root port. […]
Agree on this , the Clksrc 0 can be allocated to PCIe RP 0, it is a valid assignment.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 20:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38461/20/src/soc/intel/tigerlake/fs... File src/soc/intel/tigerlake/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/20/src/soc/intel/tigerlake/fs... PS20, Line 51: memcpy(params->SerialIoI2cMode, config->SerialIoI2cMode, Based on a comment in one of the follow-up CLs, can you ensure in all the memcpy that the copy size is the size of the destination buffer to avoid buffer overflow
_Static_assert(sizeof(dest) <= sizeof(src), "Dest buffer is > src"); memcpy(dest, src, sizeof(dest));
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 20:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38461/20/src/soc/intel/tigerlake/fs... File src/soc/intel/tigerlake/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/20/src/soc/intel/tigerlake/fs... PS20, Line 51: memcpy(params->SerialIoI2cMode, config->SerialIoI2cMode,
Based on a comment in one of the follow-up CLs, can you ensure in all the memcpy that the copy size […]
Checked on this, I could see all the memcpy size are tied with Kconfigs(LPSS and Pcie configs) and Macro(Audio). And it matches with the fsp upd array size. Do we want to put an explicit check for all memcpy? Please let me know on your thoughts.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 20:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38461/20/src/soc/intel/tigerlake/fs... File src/soc/intel/tigerlake/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/20/src/soc/intel/tigerlake/fs... PS20, Line 51: memcpy(params->SerialIoI2cMode, config->SerialIoI2cMode,
Checked on this, I could see all the memcpy size are tied with Kconfigs(LPSS and Pcie configs) and M […]
Thank you for manually checking it. The thing is there are multiple places(FSP header files and SoC chip.h) and multiple configs to check. Also things might change in the future because TGL and JSL are sharing some code. From that perspective it is error prone. Hence it does not hurt to put an explicit error check.
Aamir Bohra has uploaded a new patch set (#21) to the change originally created by Maulik V Vaghela. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
soc/intel/tigerlake: Update fsp params for Jasper Lake
Update fsp parameters for various configurations like: - graphics - usb - pci root ports - sd card - emmc - audio - basic uart configuration etc.
These are the initial settings for JSL This patch also corrects the debug_interface_flag definitions.
BUG=None BRANCH=None TEST=Compilation for jasper lake board is working
Change-Id: Ia8e88f92989fe40d7bd1c28947e005cc0d862fcb Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/soc/intel/tigerlake/chip.h M src/soc/intel/tigerlake/fsp_params_jsl.c M src/soc/intel/tigerlake/include/soc/pci_devs.h M src/soc/intel/tigerlake/romstage/fsp_params_jsl.c M src/soc/intel/tigerlake/romstage/fsp_params_tgl.c 5 files changed, 276 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/38461/21
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 21:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38461/20/src/soc/intel/tigerlake/fs... File src/soc/intel/tigerlake/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/20/src/soc/intel/tigerlake/fs... PS20, Line 51: memcpy(params->SerialIoI2cMode, config->SerialIoI2cMode,
Thank you for manually checking it. […]
Agree, added the check.
Aamir Bohra has uploaded a new patch set (#22) to the change originally created by Maulik V Vaghela. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
soc/intel/tigerlake: Update fsp params for Jasper Lake
Update fsp parameters for various configurations like: - graphics - usb - pci root ports - sd card - emmc - audio - basic uart configuration etc.
These are the initial settings for JSL This patch also corrects the debug_interface_flag definitions.
BUG=None BRANCH=None TEST=Compilation for jasper lake board is working
Change-Id: Ia8e88f92989fe40d7bd1c28947e005cc0d862fcb Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/soc/intel/tigerlake/chip.h M src/soc/intel/tigerlake/fsp_params_jsl.c M src/soc/intel/tigerlake/include/soc/pci_devs.h M src/soc/intel/tigerlake/romstage/fsp_params_jsl.c M src/soc/intel/tigerlake/romstage/fsp_params_tgl.c 5 files changed, 276 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/38461/22
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 22: Code-Review+2
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 22:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/ch... File src/soc/intel/tigerlake/chip.h:
https://review.coreboot.org/c/coreboot/+/38461/16/src/soc/intel/tigerlake/ch... PS16, Line 208: enum { : DEBUG_INTERFACE_RAM = 0x1, : DEBUG_INTERFACE_UART = 0x2, : DEBUG_INTERFACE_USB3 = 0x4, : DEBUG_INTERFACE_SERIAL_IO = 0x8, : DEBUG_INTERFACE_TRACEHUB = 0x10 : } debug_interface_flag;
Sorry , my bad, Updated the flags, the dsc description now holds valid too.
Done
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 22:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/fs... File src/soc/intel/tigerlake/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/fs... PS10, Line 138: params->SdCardGpioCmdPadTermination = 0x1F;
Set the PU/PD configuration for SD card CMD GPIO PAD, agree needs to be moved to main board gpio con […]
Done
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/pci_devs.h:
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/in... PS10, Line 125: #define PCH_DEV_SLOT_STORAGE 0x1a : #define PCH_DEVFN_EMMC _PCH_DEVFN(STORAGE, 0) : #define PCH_DEV_EMMC _PCH_DEV(STORAGE, 0)
No, guarded for JSL now.
Done
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/ro... File src/soc/intel/tigerlake/romstage/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/10/src/soc/intel/tigerlake/ro... PS10, Line 80: temporary
This was a wrong comment, we are not setting the override UPD(CpuRatioOverride) here, instead settin […]
Done
Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 22: Code-Review+1
V Sowmya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 22: Code-Review+2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 22: Code-Review+2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 22:
@Furquan, do you have any further question before merge ?
Ronak Kanabar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 22: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update fsp params for Jasper Lake ......................................................................
Patch Set 22: Code-Review-1
(19 comments)
Sorry, found some more nits.
https://review.coreboot.org/c/coreboot/+/38461/22//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38461/22//COMMIT_MSG@7 PS22, Line 7: fsp FSP
https://review.coreboot.org/c/coreboot/+/38461/22//COMMIT_MSG@9 PS22, Line 9: fsp FSP
https://review.coreboot.org/c/coreboot/+/38461/22//COMMIT_MSG@19 PS22, Line 19: This patch also corrects the debug_interface_flag definitions. Please add a blank line above.
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/ch... File src/soc/intel/tigerlake/chip.h:
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/ch... PS22, Line 207: /* Debug interface flag */ It’s clear from the name itself.
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/ch... PS22, Line 209: One space.
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/ch... PS22, Line 212: One space.
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/fs... File src/soc/intel/tigerlake/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/fs... PS22, Line 52: ARRAY_SIZE(config->SerialIoI2cMode), "copy buffer overflow!"); Can a better error message be printed?
Number of members in params and config differ
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/fs... PS22, Line 82: int unsigned int
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/fs... PS22, Line 99: params->PeiGraphicsPeimInit = 0; Please use the ternary operator.
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/fs... PS22, Line 109: Bytes lowercase: bytes
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/fs... PS22, Line 114: params->Enable8254ClockGatingOnS3 = 1; In other commits/change-sets this also uses `!CONFIG_USE_LEGACY_8254_TIMER`.
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/fs... PS22, Line 128: } Could such things be common code?
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/fs... PS22, Line 150: params->ScsSdCardEnabled = dev->enabled; Ternary operator:
params->ScsSdCardEnabled = pcidev_path_on_root(PCH_DEVFN_SDCARD) ? dev-enabled : 0;
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/fs... PS22, Line 166: dev->enabled = 0; Why doesn’t `dev` need to be checked for being present?
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/fs... PS22, Line 172: Fsp FSP
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/ro... File src/soc/intel/tigerlake/romstage/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/ro... PS22, Line 4: * Copyright (C) 2019-2020 Intel Corporation. Remove the dot/period at the end if you use the full word.
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/ro... PS22, Line 34: * is disable in devicetree.cb. Should fit in one line.
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/ro... PS22, Line 73: Cpu CPU
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/ro... PS22, Line 98: m_cfg->PchHdaEnable = dev->enabled; Please use the ternary operator.
Aamir Bohra has uploaded a new patch set (#23) to the change originally created by Maulik V Vaghela. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update FSP params for Jasper Lake ......................................................................
soc/intel/tigerlake: Update FSP params for Jasper Lake
Update FSP parameters for various configurations like: - graphics - usb - pci root ports - sd card - emmc - audio - basic uart configuration etc.
These are the initial settings for JSL
This patch also corrects the debug_interface_flag definitions.
BUG=None BRANCH=None TEST=Compilation for jasper lake board is working
Change-Id: Ia8e88f92989fe40d7bd1c28947e005cc0d862fcb Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/soc/intel/tigerlake/chip.h M src/soc/intel/tigerlake/fsp_params_jsl.c M src/soc/intel/tigerlake/include/soc/pci_devs.h M src/soc/intel/tigerlake/romstage/fsp_params_jsl.c M src/soc/intel/tigerlake/romstage/fsp_params_tgl.c 5 files changed, 265 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/38461/23
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update FSP params for Jasper Lake ......................................................................
Patch Set 23:
(19 comments)
Patch Set 22: Code-Review-1
(19 comments)
Sorry, found some more nits.
ok, np.
https://review.coreboot.org/c/coreboot/+/38461/22//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38461/22//COMMIT_MSG@7 PS22, Line 7: fsp
FSP
Done
https://review.coreboot.org/c/coreboot/+/38461/22//COMMIT_MSG@9 PS22, Line 9: fsp
FSP
Done
https://review.coreboot.org/c/coreboot/+/38461/22//COMMIT_MSG@19 PS22, Line 19: This patch also corrects the debug_interface_flag definitions.
Please add a blank line above.
Done
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/ch... File src/soc/intel/tigerlake/chip.h:
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/ch... PS22, Line 207: /* Debug interface flag */
It’s clear from the name itself.
Done
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/ch... PS22, Line 209:
One space.
Done
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/ch... PS22, Line 212:
One space.
Done
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/fs... File src/soc/intel/tigerlake/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/fs... PS22, Line 52: ARRAY_SIZE(config->SerialIoI2cMode), "copy buffer overflow!");
Can a better error message be printed? […]
I think this communicates well on compilation failure.
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/fs... PS22, Line 82: int
unsigned int
Done
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/fs... PS22, Line 99: params->PeiGraphicsPeimInit = 0;
Please use the ternary operator.
since too many conditions, I think if else hold good.
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/fs... PS22, Line 109: Bytes
lowercase: bytes
Done
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/fs... PS22, Line 114: params->Enable8254ClockGatingOnS3 = 1;
In other commits/change-sets this also uses `!CONFIG_USE_LEGACY_8254_TIMER`.
want this to be explicitly set.
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/fs... PS22, Line 128: }
Could such things be common code?
can we done, but not in scope of this change. we can have a separate CL for it.
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/fs... PS22, Line 150: params->ScsSdCardEnabled = dev->enabled;
Ternary operator: […]
Done
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/fs... PS22, Line 166: dev->enabled = 0;
Why doesn’t `dev` need to be checked for being present?
Thanks!!. Done
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/fs... PS22, Line 172: Fsp
FSP
Done
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/ro... File src/soc/intel/tigerlake/romstage/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/ro... PS22, Line 4: * Copyright (C) 2019-2020 Intel Corporation.
Remove the dot/period at the end if you use the full word.
Done
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/ro... PS22, Line 34: * is disable in devicetree.cb.
Should fit in one line.
Done
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/ro... PS22, Line 73: Cpu
CPU
Done
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/ro... PS22, Line 98: m_cfg->PchHdaEnable = dev->enabled;
Please use the ternary operator.
Done
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update FSP params for Jasper Lake ......................................................................
Patch Set 23:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38461/23/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/pci_devs.h:
https://review.coreboot.org/c/coreboot/+/38461/23/src/soc/intel/tigerlake/in... PS23, Line 90: XHCI This is confusing, why XHCI?
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update FSP params for Jasper Lake ......................................................................
Patch Set 23:
(4 comments)
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/fs... File src/soc/intel/tigerlake/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/fs... PS22, Line 52: ARRAY_SIZE(config->SerialIoI2cMode), "copy buffer overflow!");
I think this communicates well on compilation failure.
As you want.
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/fs... PS22, Line 93: respectively *accordingly* might be more appropriate.
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/fs... PS22, Line 99: params->PeiGraphicsPeimInit = 0;
since too many conditions, I think if else hold good.
Done
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/fs... PS22, Line 128: }
can we done, but not in scope of this change. we can have a separate CL for it.
Sure.
Aamir Bohra has uploaded a new patch set (#24) to the change originally created by Maulik V Vaghela. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update FSP params for Jasper Lake ......................................................................
soc/intel/tigerlake: Update FSP params for Jasper Lake
Update FSP parameters for various configurations like: - graphics - usb - pci root ports - sd card - emmc - audio - basic uart configuration etc.
These are the initial settings for JSL
This patch also corrects the debug_interface_flag definitions.
BUG=None BRANCH=None TEST=Compilation for jasper lake board is working
Change-Id: Ia8e88f92989fe40d7bd1c28947e005cc0d862fcb Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/soc/intel/tigerlake/chip.h M src/soc/intel/tigerlake/fsp_params_jsl.c M src/soc/intel/tigerlake/include/soc/pci_devs.h M src/soc/intel/tigerlake/romstage/fsp_params_jsl.c M src/soc/intel/tigerlake/romstage/fsp_params_tgl.c 5 files changed, 265 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/38461/24
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update FSP params for Jasper Lake ......................................................................
Patch Set 24:
(4 comments)
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/fs... File src/soc/intel/tigerlake/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/fs... PS22, Line 52: ARRAY_SIZE(config->SerialIoI2cMode), "copy buffer overflow!");
As you want.
Done
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/fs... PS22, Line 93: respectively
*accordingly* might be more appropriate.
Done
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/fs... PS22, Line 114: params->Enable8254ClockGatingOnS3 = 1;
want this to be explicitly set.
Done
https://review.coreboot.org/c/coreboot/+/38461/22/src/soc/intel/tigerlake/fs... PS22, Line 128: }
Sure.
Done
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update FSP params for Jasper Lake ......................................................................
Patch Set 24:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38461/23/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/pci_devs.h:
https://review.coreboot.org/c/coreboot/+/38461/23/src/soc/intel/tigerlake/in... PS23, Line 90: XHCI
This is confusing, why XHCI?
because of PCH_DEV_SLOT_XHCI 0x14 this group is diverse to tie up with single notation. any suggestion? Anyways the code reference would use PCH_DEVFN_SDCARD and PCH_DEV_SDCARD for SD card reference.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update FSP params for Jasper Lake ......................................................................
Patch Set 24:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38461/24//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38461/24//COMMIT_MSG@18 PS24, Line 18: These are the initial settings for JSL super-nit: Period at the end.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update FSP params for Jasper Lake ......................................................................
Patch Set 24: Code-Review+1
(8 comments)
https://review.coreboot.org/c/coreboot/+/38461/24//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38461/24//COMMIT_MSG@10 PS24, Line 10: - graphics I would capitalize each element, as well as the acronyms:
- USB - PCI root ports - SD card - eMMC - Audio - Basic UART configuration
https://review.coreboot.org/c/coreboot/+/38461/24//COMMIT_MSG@16 PS24, Line 16: etc. nit: I would omit the "etc"
https://review.coreboot.org/c/coreboot/+/38461/24//COMMIT_MSG@24 PS24, Line 24: jasper lake nit: capitalized as `Jasper Lake` ?
https://review.coreboot.org/c/coreboot/+/38461/24/src/soc/intel/tigerlake/ch... File src/soc/intel/tigerlake/chip.h:
https://review.coreboot.org/c/coreboot/+/38461/24/src/soc/intel/tigerlake/ch... PS24, Line 208: enum { Is this is a bitfield? If so, I would use shifts.
I mean, if the enum values are used like this:
u8 some_variable = DEBUG_INTERFACE_UART | DEBUG_INTERFACE_TRACEHUB;
Then I would change the hex constants to be like this:
enum { DEBUG_INTERFACE_RAM = (1 << 0), DEBUG_INTERFACE_UART = (1 << 1), DEBUG_INTERFACE_USB3 = (1 << 3), DEBUG_INTERFACE_SERIAL_IO = (1 << 4), DEBUG_INTERFACE_TRACEHUB = (1 << 5), } debug_interface_flag;
This is functionally equivalent, but it is more self-explanatory :)
https://review.coreboot.org/c/coreboot/+/38461/24/src/soc/intel/tigerlake/fs... File src/soc/intel/tigerlake/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/24/src/soc/intel/tigerlake/fs... PS24, Line 114: 1 Shouldn't this mirror the value above?
https://review.coreboot.org/c/coreboot/+/38461/24/src/soc/intel/tigerlake/ro... File src/soc/intel/tigerlake/romstage/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/24/src/soc/intel/tigerlake/ro... PS24, Line 32: disable disable*d*
https://review.coreboot.org/c/coreboot/+/38461/24/src/soc/intel/tigerlake/ro... PS24, Line 46: pci PCI
https://review.coreboot.org/c/coreboot/+/38461/24/src/soc/intel/tigerlake/ro... PS24, Line 85: Vt-D VT-d
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update FSP params for Jasper Lake ......................................................................
Patch Set 24:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38461/24//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38461/24//COMMIT_MSG@24 PS24, Line 24: jasper lake
nit: capitalized as `Jasper Lake` ?
Or, if this Jasper Lake board is google/dedede, maybe use:
TEST=Build dedede board
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update FSP params for Jasper Lake ......................................................................
Patch Set 24:
(9 comments)
https://review.coreboot.org/c/coreboot/+/38461/24//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38461/24//COMMIT_MSG@10 PS24, Line 10: - graphics
I would capitalize each element, as well as the acronyms: […]
Done
https://review.coreboot.org/c/coreboot/+/38461/24//COMMIT_MSG@16 PS24, Line 16: etc.
nit: I would omit the "etc"
Done
https://review.coreboot.org/c/coreboot/+/38461/24//COMMIT_MSG@18 PS24, Line 18: These are the initial settings for JSL
super-nit: Period at the end.
Done
https://review.coreboot.org/c/coreboot/+/38461/24//COMMIT_MSG@24 PS24, Line 24: jasper lake
Or, if this Jasper Lake board is google/dedede, maybe use: […]
Done
https://review.coreboot.org/c/coreboot/+/38461/24/src/soc/intel/tigerlake/ch... File src/soc/intel/tigerlake/chip.h:
https://review.coreboot.org/c/coreboot/+/38461/24/src/soc/intel/tigerlake/ch... PS24, Line 208: enum {
Is this is a bitfield? If so, I would use shifts. […]
Agree. Done.
https://review.coreboot.org/c/coreboot/+/38461/24/src/soc/intel/tigerlake/fs... File src/soc/intel/tigerlake/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/24/src/soc/intel/tigerlake/fs... PS24, Line 114: 1
Shouldn't this mirror the value above?
want to explicitly set it for S3 flow.
https://review.coreboot.org/c/coreboot/+/38461/24/src/soc/intel/tigerlake/ro... File src/soc/intel/tigerlake/romstage/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/24/src/soc/intel/tigerlake/ro... PS24, Line 32: disable
disable*d*
Done
https://review.coreboot.org/c/coreboot/+/38461/24/src/soc/intel/tigerlake/ro... PS24, Line 46: pci
PCI
Done
https://review.coreboot.org/c/coreboot/+/38461/24/src/soc/intel/tigerlake/ro... PS24, Line 85: Vt-D
VT-d
Done
Aamir Bohra has uploaded a new patch set (#25) to the change originally created by Maulik V Vaghela. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update FSP params for Jasper Lake ......................................................................
soc/intel/tigerlake: Update FSP params for Jasper Lake
Update FSP parameters for various configurations like: - graphics - USB - PCIe root ports - SD card - eMMC - Audio - Basic UART configuration
These are the initial settings for JSL.
This patch also corrects the debug_interface_flag definitions.
TEST=Build dedede board
Change-Id: Ia8e88f92989fe40d7bd1c28947e005cc0d862fcb Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/soc/intel/tigerlake/chip.h M src/soc/intel/tigerlake/fsp_params_jsl.c M src/soc/intel/tigerlake/include/soc/pci_devs.h M src/soc/intel/tigerlake/romstage/fsp_params_jsl.c M src/soc/intel/tigerlake/romstage/fsp_params_tgl.c 5 files changed, 265 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/38461/25
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update FSP params for Jasper Lake ......................................................................
Patch Set 25: Code-Review+2
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update FSP params for Jasper Lake ......................................................................
Patch Set 25:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38461/24/src/soc/intel/tigerlake/ch... File src/soc/intel/tigerlake/chip.h:
https://review.coreboot.org/c/coreboot/+/38461/24/src/soc/intel/tigerlake/ch... PS24, Line 208: enum {
Agree. Done.
Done
https://review.coreboot.org/c/coreboot/+/38461/24/src/soc/intel/tigerlake/fs... File src/soc/intel/tigerlake/fsp_params_jsl.c:
https://review.coreboot.org/c/coreboot/+/38461/24/src/soc/intel/tigerlake/fs... PS24, Line 114: 1
want to explicitly set it for S3 flow.
Done
https://review.coreboot.org/c/coreboot/+/38461/23/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/pci_devs.h:
https://review.coreboot.org/c/coreboot/+/38461/23/src/soc/intel/tigerlake/in... PS23, Line 90: XHCI
because of PCH_DEV_SLOT_XHCI 0x14 […]
Ack
V Sowmya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update FSP params for Jasper Lake ......................................................................
Patch Set 25: Code-Review+2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update FSP params for Jasper Lake ......................................................................
Patch Set 25: Code-Review+2
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update FSP params for Jasper Lake ......................................................................
Patch Set 25:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38461/23/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/pci_devs.h:
https://review.coreboot.org/c/coreboot/+/38461/23/src/soc/intel/tigerlake/in... PS23, Line 90: XHCI
Ack
currently it seems like SDCARD function is under XHCI controller
you could define "PCH_DEV_SLOT_SDCARD 0x14"
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update FSP params for Jasper Lake ......................................................................
Patch Set 25: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/38461/23/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/pci_devs.h:
https://review.coreboot.org/c/coreboot/+/38461/23/src/soc/intel/tigerlake/in... PS23, Line 90: XHCI
currently it seems like SDCARD function is under XHCI controller […]
Aah, just looked at the EDS and SDcard function is under the same (DEV_SLOT) PCI device as XHCI.
Subrata Banik has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update FSP params for Jasper Lake ......................................................................
soc/intel/tigerlake: Update FSP params for Jasper Lake
Update FSP parameters for various configurations like: - graphics - USB - PCIe root ports - SD card - eMMC - Audio - Basic UART configuration
These are the initial settings for JSL.
This patch also corrects the debug_interface_flag definitions.
TEST=Build dedede board
Change-Id: Ia8e88f92989fe40d7bd1c28947e005cc0d862fcb Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/38461 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Karthik Ramasubramanian kramasub@google.com Reviewed-by: V Sowmya v.sowmya@intel.com Reviewed-by: Subrata Banik subrata.banik@intel.com Reviewed-by: Rizwan Qureshi rizwan.qureshi@intel.com --- M src/soc/intel/tigerlake/chip.h M src/soc/intel/tigerlake/fsp_params_jsl.c M src/soc/intel/tigerlake/include/soc/pci_devs.h M src/soc/intel/tigerlake/romstage/fsp_params_jsl.c M src/soc/intel/tigerlake/romstage/fsp_params_tgl.c 5 files changed, 265 insertions(+), 14 deletions(-)
Approvals: build bot (Jenkins): Verified Rizwan Qureshi: Looks good to me, approved Subrata Banik: Looks good to me, approved V Sowmya: Looks good to me, approved Karthik Ramasubramanian: Looks good to me, approved
diff --git a/src/soc/intel/tigerlake/chip.h b/src/soc/intel/tigerlake/chip.h index 75a399f..5442361 100644 --- a/src/soc/intel/tigerlake/chip.h +++ b/src/soc/intel/tigerlake/chip.h @@ -204,6 +204,15 @@ */ uint8_t SerialIoGSpiCsState[CONFIG_SOC_INTEL_COMMON_BLOCK_GSPI_MAX];
+ /* Debug interface selection */ + enum { + DEBUG_INTERFACE_RAM = (1 << 0), + DEBUG_INTERFACE_UART = (1 << 1), + DEBUG_INTERFACE_USB3 = (1 << 3), + DEBUG_INTERFACE_SERIAL_IO = (1 << 4), + DEBUG_INTERFACE_TRACEHUB = (1 << 5), + } debug_interface_flag; + /* GPIO SD card detect pin */ unsigned int sdcard_cd_gpio;
diff --git a/src/soc/intel/tigerlake/fsp_params_jsl.c b/src/soc/intel/tigerlake/fsp_params_jsl.c index 6fb2f9f..8eb3fba 100644 --- a/src/soc/intel/tigerlake/fsp_params_jsl.c +++ b/src/soc/intel/tigerlake/fsp_params_jsl.c @@ -1,7 +1,7 @@ /* * This file is part of the coreboot project. * - * Copyright (C) 2019 Intel Corporation. + * Copyright (C) 2019-2020 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 @@ -12,10 +12,19 @@ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * GNU General Public License for more details. */ - +#include <assert.h> +#include <console/console.h> #include <fsp/api.h> +#include <fsp/ppi/mp_service_ppi.h> +#include <fsp/util.h> #include <intelblocks/lpss.h> +#include <intelblocks/mp_init.h> +#include <intelblocks/xdci.h> +#include <soc/intel/common/vbt.h> +#include <soc/pci_devs.h> #include <soc/ramstage.h> +#include <soc/soc_chip.h> +#include <string.h>
static const pci_devfn_t serial_io_dev[] = { PCH_DEVFN_I2C0, @@ -32,10 +41,138 @@ PCH_DEVFN_UART2 };
+static void parse_devicetree(FSP_S_CONFIG *params) +{ + const struct soc_intel_tigerlake_config *config = config_of_soc(); + + /* LPSS controllers configuration */ + + /* I2C */ + _Static_assert(ARRAY_SIZE(params->SerialIoI2cMode) >= + ARRAY_SIZE(config->SerialIoI2cMode), "copy buffer overflow!"); + memcpy(params->SerialIoI2cMode, config->SerialIoI2cMode, + sizeof(config->SerialIoI2cMode)); + + /* GSPI */ + _Static_assert(ARRAY_SIZE(params->SerialIoSpiMode) >= + ARRAY_SIZE(config->SerialIoGSpiMode), "copy buffer overflow!"); + memcpy(params->SerialIoSpiMode, config->SerialIoGSpiMode, + sizeof(config->SerialIoGSpiMode)); + + _Static_assert(ARRAY_SIZE(params->SerialIoSpiCsMode) >= + ARRAY_SIZE(config->SerialIoGSpiCsMode), "copy buffer overflow!"); + memcpy(params->SerialIoSpiCsMode, config->SerialIoGSpiCsMode, + sizeof(config->SerialIoGSpiCsMode)); + + _Static_assert(ARRAY_SIZE(params->SerialIoSpiCsState) >= + ARRAY_SIZE(config->SerialIoGSpiCsState), "copy buffer overflow!"); + memcpy(params->SerialIoSpiCsState, config->SerialIoGSpiCsState, + sizeof(config->SerialIoGSpiCsState)); + + /* UART */ + _Static_assert(ARRAY_SIZE(params->SerialIoUartMode) >= + ARRAY_SIZE(config->SerialIoUartMode), "copy buffer overflow!"); + memcpy(params->SerialIoUartMode, config->SerialIoUartMode, + sizeof(config->SerialIoUartMode)); +} + /* UPD parameters to be initialized before SiliconInit */ void platform_fsp_silicon_init_params_cb(FSPS_UPD *supd) { - /* TODO: Update with UPD override as FSP matures */ + unsigned int i; + struct device *dev; + FSP_S_CONFIG *params = &supd->FspsConfig; + struct soc_intel_tigerlake_config *config = config_of_soc(); + + /* Parse device tree and fill in FSP UPDs */ + parse_devicetree(params); + + /* Load VBT before devicetree-specific config. */ + params->GraphicsConfigPtr = (uintptr_t)vbt_get(); + + /* Check if IGD is present and fill Graphics init param accordingly */ + dev = pcidev_path_on_root(SA_DEVFN_IGD); + + if (CONFIG(RUN_FSP_GOP) && dev && dev->enabled) + params->PeiGraphicsPeimInit = 1; + else + params->PeiGraphicsPeimInit = 0; + + /* Use coreboot MP PPI services if Kconfig is enabled */ + if (CONFIG(USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI)) { + params->CpuMpPpi = (uintptr_t) mp_fill_ppi_services_data(); + params->SkipMpInit = 0; + } else { + params->SkipMpInit = !CONFIG_USE_INTEL_FSP_MP_INIT; + } + + /* Unlock upper 8 bytes of RTC RAM */ + params->RtcMemoryLock = 0; + + /* Legacy 8254 timer support */ + params->Enable8254ClockGating = !CONFIG_USE_LEGACY_8254_TIMER; + params->Enable8254ClockGatingOnS3 = 1; + + /* disable Legacy PME */ + memset(params->PcieRpPmSci, 0, sizeof(params->PcieRpPmSci)); + + /* USB configuration */ + for (i = 0; i < ARRAY_SIZE(config->usb2_ports); i++) { + + params->PortUsb20Enable[i] = config->usb2_ports[i].enable; + params->Usb2OverCurrentPin[i] = config->usb2_ports[i].ocpin; + params->Usb2PhyPetxiset[i] = config->usb2_ports[i].pre_emp_bias; + params->Usb2PhyTxiset[i] = config->usb2_ports[i].tx_bias; + params->Usb2PhyPredeemp[i] = config->usb2_ports[i].tx_emp_enable; + params->Usb2PhyPehalfbit[i] = config->usb2_ports[i].pre_emp_bit; + } + + for (i = 0; i < ARRAY_SIZE(config->usb3_ports); i++) { + + params->PortUsb30Enable[i] = config->usb3_ports[i].enable; + params->Usb3OverCurrentPin[i] = config->usb3_ports[i].ocpin; + if (config->usb3_ports[i].tx_de_emp) { + params->Usb3HsioTxDeEmphEnable[i] = 1; + params->Usb3HsioTxDeEmph[i] = config->usb3_ports[i].tx_de_emp; + } + if (config->usb3_ports[i].tx_downscale_amp) { + params->Usb3HsioTxDownscaleAmpEnable[i] = 1; + params->Usb3HsioTxDownscaleAmp[i] = + config->usb3_ports[i].tx_downscale_amp; + } + } + + /* SDCard related configuration */ + params->ScsSdCardEnabled = pcidev_path_on_root(PCH_DEVFN_SDCARD) ? dev->enabled : 0; + + params->Device4Enable = config->Device4Enable; + + /* eMMC configuration */ + dev = pcidev_path_on_root(PCH_DEVFN_EMMC); + if (!dev) { + params->ScsEmmcEnabled = 0; + } else { + params->ScsEmmcEnabled = dev->enabled; + params->ScsEmmcHs400Enabled = config->ScsEmmcHs400Enabled; + } + + /* Enable xDCI controller if enabled in devicetree and allowed */ + dev = pcidev_path_on_root(PCH_DEVFN_USBOTG); + if (!dev || !xdci_can_enable()) + dev->enabled = 0; + params->XdciEnable = dev->enabled; + + /* Provide correct UART number for FSP debug logs */ + params->SerialIoDebugUartNumber = CONFIG_UART_FOR_CONSOLE; + + /* Override/Fill FSP Silicon Param for mainboard */ + mainboard_silicon_init_params(params); +} + +/* Mainboard GPIO Configuration */ +__weak void mainboard_silicon_init_params(FSP_S_CONFIG *params) +{ + printk(BIOS_DEBUG, "WEAK: %s/%s called\n", __FILE__, __func__); }
/* Return list of SOC LPSS controllers */ diff --git a/src/soc/intel/tigerlake/include/soc/pci_devs.h b/src/soc/intel/tigerlake/include/soc/pci_devs.h index 9a35e73..ef2dfe3 100644 --- a/src/soc/intel/tigerlake/include/soc/pci_devs.h +++ b/src/soc/intel/tigerlake/include/soc/pci_devs.h @@ -86,6 +86,11 @@ #define PCH_DEV_SRAM _PCH_DEV(XHCI, 2) #define PCH_DEV_CNVI_WIFI _PCH_DEV(XHCI, 3)
+#if CONFIG(SOC_INTEL_JASPERLAKE) +#define PCH_DEVFN_SDCARD _PCH_DEVFN(XHCI, 5) +#define PCH_DEV_SDCARD _PCH_DEV(XHCI, 5) +#endif + #define PCH_DEV_SLOT_SIO3 0x15 #define PCH_DEVFN_I2C0 _PCH_DEVFN(SIO3, 0) #define PCH_DEVFN_I2C1 _PCH_DEVFN(SIO3, 1) @@ -122,6 +127,12 @@ #define PCH_DEV_I2C5 _PCH_DEV(SIO4, 1) #define PCH_DEV_UART2 _PCH_DEV(SIO4, 2)
+#if CONFIG(SOC_INTEL_JASPERLAKE) +#define PCH_DEV_SLOT_STORAGE 0x1a +#define PCH_DEVFN_EMMC _PCH_DEVFN(STORAGE, 0) +#define PCH_DEV_EMMC _PCH_DEV(STORAGE, 0) +#endif + #define PCH_DEV_SLOT_PCIE 0x1c #define PCH_DEVFN_PCIE1 _PCH_DEVFN(PCIE, 0) #define PCH_DEVFN_PCIE2 _PCH_DEVFN(PCIE, 1) diff --git a/src/soc/intel/tigerlake/romstage/fsp_params_jsl.c b/src/soc/intel/tigerlake/romstage/fsp_params_jsl.c index 810cff4..e88d809 100644 --- a/src/soc/intel/tigerlake/romstage/fsp_params_jsl.c +++ b/src/soc/intel/tigerlake/romstage/fsp_params_jsl.c @@ -1,7 +1,7 @@ /* * This file is part of the coreboot project. * - * Copyright (C) 2019 Intel Corp. + * Copyright (C) 2019-2020 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 @@ -13,10 +13,113 @@ * GNU General Public License for more details. */
+#include <assert.h> +#include <console/console.h> #include <fsp/util.h> +#include <soc/pci_devs.h> #include <soc/romstage.h> +#include <soc/soc_chip.h> +#include <string.h> + +static void soc_memory_init_params(FSP_M_CONFIG *m_cfg, + const struct soc_intel_tigerlake_config *config) +{ + unsigned int i; + const struct device *dev = pcidev_path_on_root(SA_DEVFN_IGD); + uint32_t mask = 0; + + if (!dev || !dev->enabled) { + /* Skip IGD initialization in FSP if device is disabled in devicetree.cb */ + m_cfg->InternalGfx = 0; + m_cfg->IgdDvmt50PreAlloc = 0; + } else { + m_cfg->InternalGfx = 1; + /* Set IGD stolen size to 60MB. */ + m_cfg->IgdDvmt50PreAlloc = 0xFE; + } + + m_cfg->TsegSize = CONFIG_SMM_TSEG_SIZE; + m_cfg->IedSize = CONFIG_IED_REGION_SIZE; + m_cfg->SaGv = config->SaGv; + m_cfg->RMT = config->RMT; + + /* PCIe root port configuration */ + for (i = 0; i < ARRAY_SIZE(config->PcieRpEnable); i++) { + if (config->PcieRpEnable[i]) + mask |= (1 << i); + } + + m_cfg->PcieRpEnableMask = mask; + + _Static_assert(ARRAY_SIZE(m_cfg->PcieClkSrcUsage) >= + ARRAY_SIZE(config->PcieClkSrcUsage), "copy buffer overflow!"); + memcpy(m_cfg->PcieClkSrcUsage, config->PcieClkSrcUsage, + sizeof(config->PcieClkSrcUsage)); + + _Static_assert(ARRAY_SIZE(m_cfg->PcieClkSrcClkReq) >= + ARRAY_SIZE(config->PcieClkSrcClkReq), "copy buffer overflow!"); + memcpy(m_cfg->PcieClkSrcClkReq, config->PcieClkSrcClkReq, + sizeof(config->PcieClkSrcClkReq)); + + m_cfg->PrmrrSize = config->PrmrrSize; + m_cfg->EnableC6Dram = config->enable_c6dram; + + /* Disable BIOS Guard */ + m_cfg->BiosGuard = 0; + + /* Set CPU Ratio */ + m_cfg->CpuRatio = 0; + m_cfg->PcdDebugInterfaceFlags = CONFIG(DRIVERS_UART_8250IO) ? + DEBUG_INTERFACE_UART : DEBUG_INTERFACE_TRACEHUB; + + /* Change VmxEnable UPD value according to ENABLE_VMX Kconfig */ + m_cfg->VmxEnable = CONFIG(ENABLE_VMX); + + + /* Enable SMBus controller based on config */ + m_cfg->SmbusEnable = config->SmbusEnable; + + /* Set debug probe type */ + m_cfg->PlatformDebugConsent = config->DebugConsent; + + /* VT-d config */ + m_cfg->VtdDisable = 0; + + m_cfg->SerialIoUartDebugControllerNumber = CONFIG_UART_FOR_CONSOLE; + + /* Audio */ + m_cfg->PchHdaEnable = pcidev_path_on_root(PCH_DEVFN_HDA) ? dev->enabled : 0; + + m_cfg->PchHdaDspEnable = config->PchHdaDspEnable; + m_cfg->PchHdaAudioLinkHdaEnable = config->PchHdaAudioLinkHdaEnable; + + _Static_assert(ARRAY_SIZE(m_cfg->PchHdaAudioLinkDmicEnable) >= + ARRAY_SIZE(config->PchHdaAudioLinkDmicEnable), "copy buffer overflow!"); + memcpy(m_cfg->PchHdaAudioLinkDmicEnable, config->PchHdaAudioLinkDmicEnable, + sizeof(config->PchHdaAudioLinkDmicEnable)); + + _Static_assert(ARRAY_SIZE(m_cfg->PchHdaAudioLinkSspEnable) >= + ARRAY_SIZE(config->PchHdaAudioLinkSspEnable), "copy buffer overflow!"); + memcpy(m_cfg->PchHdaAudioLinkSspEnable, config->PchHdaAudioLinkSspEnable, + sizeof(config->PchHdaAudioLinkSspEnable)); + + _Static_assert(ARRAY_SIZE(m_cfg->PchHdaAudioLinkSndwEnable) >= + ARRAY_SIZE(config->PchHdaAudioLinkSndwEnable), "copy buffer overflow!"); + memcpy(m_cfg->PchHdaAudioLinkSndwEnable, config->PchHdaAudioLinkSndwEnable, + sizeof(config->PchHdaAudioLinkSndwEnable)); +}
void platform_fsp_memory_init_params_cb(FSPM_UPD *mupd, uint32_t version) { - /* TODO: Update with UPD override as FSP matures */ + const struct soc_intel_tigerlake_config *config = config_of_soc(); + FSP_M_CONFIG *m_cfg = &mupd->FspmConfig; + + soc_memory_init_params(m_cfg, config); + + mainboard_memory_init_params(mupd); +} + +__weak void mainboard_memory_init_params(FSPM_UPD *mupd) +{ + printk(BIOS_DEBUG, "WEAK: %s/%s called\n", __FILE__, __func__); } diff --git a/src/soc/intel/tigerlake/romstage/fsp_params_tgl.c b/src/soc/intel/tigerlake/romstage/fsp_params_tgl.c index 8b32bc0..ed6aa5a 100644 --- a/src/soc/intel/tigerlake/romstage/fsp_params_tgl.c +++ b/src/soc/intel/tigerlake/romstage/fsp_params_tgl.c @@ -23,15 +23,6 @@ #include <soc/soc_chip.h> #include <string.h>
-/* Debug interface flag */ -enum debug_interface_flag { - DEBUG_INTERFACE_RAM = 0x1, - DEBUG_INTERFACE_UART = 0x2, - DEBUG_INTERFACE_USB3 = 0x4, - DEBUG_INTERFACE_SERIAL_IO = 0x8, - DEBUG_INTERFACE_TRACEHUB = 0x10 -}; - static void soc_memory_init_params(FSP_M_CONFIG *m_cfg, const struct soc_intel_tigerlake_config *config) {
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38461 )
Change subject: soc/intel/tigerlake: Update FSP params for Jasper Lake ......................................................................
Patch Set 26:
Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3 Emulation targets: EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/914 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/913 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/912
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Aamir Bohra (Code Review)
-
Angel Pons (Code Review)
-
build bot (Jenkins) (Code Review)
-
Furquan Shaikh (Code Review)
-
Karthik Ramasubramanian (Code Review)
-
Maulik V Vaghela (Code Review)
-
Paul Menzel (Code Review)
-
Rizwan Qureshi (Code Review)
-
Ronak Kanabar (Code Review)
-
Subrata Banik (Code Review)
-
V Sowmya (Code Review)
-
Wonkyu Kim (Code Review)