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.