Attention is currently required from: Arthur Heymans, Nico Huber, Maulik V Vaghela, Paul Menzel, Mario Scheithauer, Angel Pons, Subrata Banik, Michael Niewöhner, Werner Zeh, Patrick Rudolph, Felix Held. Lean Sheng Tan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55367 )
Change subject: soc/intel/elkhartlake: Introduce Intel PSE ......................................................................
Patch Set 50:
(13 comments)
Patchset:
PS46:
@Nico I wonder how you want to integrate that minimal sample exactly. […]
Yes, this UPD has already been set to 0 if PSE is disabled. /* PSE (Intel Programmable Services Engine) switch */ m_cfg->PchPseEnable = CONFIG(PSE_ENABLE);
File src/soc/intel/elkhartlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/55367/comment/1f617253_7869a3df PS49, Line 203: be
*to* be?
Done
https://review.coreboot.org/c/coreboot/+/55367/comment/8f5e1a69_6adc62f0 PS49, Line 202: that is designed as an Asymmetric : Multi-Processing (AMP) system
AIUI, Intel describes the whole SoC as AMP. Some cores are in the CPU complex and […]
Yes Nico you are right :) Done reworked.
https://review.coreboot.org/c/coreboot/+/55367/comment/b863b2fa_7e41ca30 PS49, Line 204: added to CBFS and loaded by FSP to run PSE.
Maybe a misunderstanding. We don't explicitly mention that a ramstage binary […]
Done reworded.
File src/soc/intel/elkhartlake/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/55367/comment/cbdb438e_6952e160 PS49, Line 52: CONFIG_PSE_IMAGE
Did you mean CONFIG_PSE_ENABLE?
Done
https://review.coreboot.org/c/coreboot/+/55367/comment/c0b71471_1ee05877 PS49, Line 66:
Well it's not the common coreboot practice. This is not the […]
Valid concern here thanks. Added ADD_PSE_IMAGE_TO_CBFS config to guard the FW file.
File src/soc/intel/elkhartlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/55367/comment/d2fe9768_68feb560 PS49, Line 383: /* PSE devices config */
I don't understand this comment.
Done reworked.
https://review.coreboot.org/c/coreboot/+/55367/comment/136a6b79_8990302f PS49, Line 384: /* PSE DMA */
That's literally what the indentifier says, such a comment is just distraction.
Done removed all of them.
https://review.coreboot.org/c/coreboot/+/55367/comment/5836ac3c_42697ae1 PS49, Line 423:
Nit, missing line break after the /*
Done
https://review.coreboot.org/c/coreboot/+/55367/comment/ff89601c_5b2ffb32 PS49, Line 426: * as it still allows user to change these params from devicetree.
Yes, this could be written more precise her. […]
Done reworked the wordings.
https://review.coreboot.org/c/coreboot/+/55367/comment/1796a6e0_a4f8113d PS49, Line 436: 0x0
Why is this hex?
Done
https://review.coreboot.org/c/coreboot/+/55367/comment/013db742_edf5a34b PS49, Line 438: 0x3
Why is this hex?
Done
https://review.coreboot.org/c/coreboot/+/55367/comment/b9497f0f_e8a293f5 PS49, Line 438: //
Please use common comment style.
Done
-
Lean Sheng Tan (Code Review)