Attention is currently required from: Angel Pons, Arthur Heymans, Christian Walter, Jincheng Li, Lean Sheng Tan, Mark Chang, Patrick Rudolph.
Angel Pons has posted comments on this change by Mark Chang. ( https://review.coreboot.org/c/coreboot/+/85532?usp=email )
Change subject: Add support for MiTAC Computing Whitestone-2 mainboard ......................................................................
Patch Set 1: Code-Review+1
(12 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/85532/comment/7f2a0013_aa001d5a?usp... : PS1, Line 8: Could you please add a body to the commit message? I would suggest describing the board (looks like it's a single-socket SPR-SP evaluation board) as well as what has been tested. You can use CB:68188 as an example of this structure.
File configs/config.mitaccomputing_ws_2:
PS1: I see references to site-local here. This config file will get build-tested by Jenkins, so this won't work. But if Jenkins passed, I guess it didn't pick up the file (IIRC, if the beginning of the filename doesn't match the mainboard vendor/name in the way Jenkins expects it, then the file is ignored).
Ironically enough, files in `configs/builder/` do *not* get build-tested (unless this has changed). So you can move this config file into `configs/builder/`.
https://review.coreboot.org/c/coreboot/+/85532/comment/bee674c2_4920f20a?usp... : PS1, Line 15: CONFIG_X2APIC_LATE_WORKAROUND=y Is this needed for all SPR-SP mainboards?
File src/mainboard/mitaccomputing/whitestone-2/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/85532/comment/22b8ce6b_aa1fc738?usp... : PS1, Line 30:
Suppose the sleep/wake is not supported so no needed?
I think these methods might be needed for ASL to compile, but I would prefer removing them if possible.
File src/mainboard/mitaccomputing/whitestone-2/board_info.txt:
PS1: nit: Missing `ROM package`
File src/mainboard/mitaccomputing/whitestone-2/bootblock.c:
https://review.coreboot.org/c/coreboot/+/85532/comment/75eb449d_38a09f35?usp... : PS1, Line 18: static void enable_espi_lpc_io_windows(void)
Is is possible to use the lpc functions in `src/soc/intel/common/block/lpc/lpc_lib. […]
I think that library is for ramstage. Also, other Xeon-SP boards do this.
Still, I agree it would be nice to tidy this up, but I feel it would be best to do so in a follow-up.
File src/mainboard/mitaccomputing/whitestone-2/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/85532/comment/2e1a2b8c_038438ee?usp... : PS1, Line 5: register "turbo_ratio_limit" = "0x181819191e242424" : register "turbo_ratio_limit_cores" = "0x3836322e2a1c1a18" Is this hardcoded for a specific CPU model?
File src/mainboard/mitaccomputing/whitestone-2/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/85532/comment/f9ae236c_fc002295?usp... : PS1, Line 10: 0x20110725 // OEM revision The "OEM revision" comment means this number was chosen to match that of vendor firmware (BIOS/UEFI) for a particular board (on a retrofitted coreboot port). Unfortunately, this got copy-pasted verbatim to other boards without checking if the comment is true.
It would be great if you can check this value. But if you can't check it, please remove the comment:
```suggestion 0x20110725 ```
File src/mainboard/mitaccomputing/whitestone-2/include/sprsp_ws_2_iio.h:
https://review.coreboot.org/c/coreboot/+/85532/comment/0c5c82e2_2653ccc0?usp... : PS1, Line 48: CFG_UPD_PCIE_PORT(1, 0, 0) Idea: you could add this macro ``` #define CFG_UPD_PCIE_PORT_DISABLED CFG_UPD_PCIE_PORT(1, 0, 0) ``` and then do ```suggestion CFG_UPD_PCIE_PORT_DISABLED, ``` so that it's easier to see which ports are enabled/disabled.
File src/mainboard/mitaccomputing/whitestone-2/mainboard.c:
https://review.coreboot.org/c/coreboot/+/85532/comment/b6406a31_2932baa5?usp... : PS1, Line 46: }
Can the common codes be used? smbios_write_type41
These E810 devices are Ethernet NICs. I think they are on-board and you want them to have consistent numbering in the OS, is this correct?
The simplest thing you can do is declare them in the devicetree, so that coreboot autogenerates SMBIOS Type41 entries for them.
If you need to assign specific numbers to specific devices, you can do something like CB:58628 to be able to manually specify numbers.
https://review.coreboot.org/c/coreboot/+/85532/comment/c2993165_227d047e?usp... : PS1, Line 80: };
How this be invoked?
It's invoked automatically
File src/mainboard/mitaccomputing/whitestone-2/romstage.c:
https://review.coreboot.org/c/coreboot/+/85532/comment/aed8bda5_ce1fcfd8?usp... : PS1, Line 18: Protocl typo ```suggestion /* eg. Protocol Auto Negotiation */ ```