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