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