Attention is currently required from: Mariusz Szafrański, Paul Menzel, Suresh Bellampalli, Vanessa Eusebio, Michal Motyl, Kyösti Mälkki, Patrick Rudolph, King Sumo. Дмитрий Понаморев has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/57194 )
Change subject: src/mainboard/teleplatforms/D4E4S16P8: Add new CRB teleplatforms/D4E4S16P8 folder. ......................................................................
Patch Set 16:
(38 comments)
This change is ready for review.
Patchset:
PS16: Eliminated the controversial code and the code related to the video adapter (Aspeed). Hopefully I'll add the necessary changes later.
File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/57194/comment/a6c1db2d_0bb3fb7e PS13, Line 169: Always uncondtionally run the option regardless of other
'uncondtionally' may be misspelled - perhaps 'unconditionally'?
Please fix.
File src/device/pci_dump.c:
https://review.coreboot.org/c/coreboot/+/57194/comment/c8cf68f4_cd5ab9b4 PS7, Line 12: */
Use SPDX header?
Done
File src/include/device/pci.h:
https://review.coreboot.org/c/coreboot/+/57194/comment/0399dcc5_c6bf6cc4 PS7, Line 109: #if CONFIG(ASPEED_SOFT_STRAP_NEEDED)
Please separate adding support for ASPEED_SOFT_STRAP_NEEDED out into a separate commit/change-set?
Good. I will add patches in the future.
File src/mainboard/teleplatforms/D4E4S16P8/ast25xx.c:
https://review.coreboot.org/c/coreboot/+/57194/comment/13d2d986_d64a2861 PS8, Line 23: #define ASPEED_RP_NUM 0x11 // 17
please, no space before tabs
Please fix.
https://review.coreboot.org/c/coreboot/+/57194/comment/bd4d3c94_3c42a044 PS8, Line 140: if (class == PCI_CLASS_DISPLAY_VGA) {
braces {} are not necessary for single statement blocks
Please fix.
https://review.coreboot.org/c/coreboot/+/57194/comment/8fb5b196_9ca42deb PS8, Line 180: mdelay(500);
trailing whitespace
Please fix.
File src/mainboard/teleplatforms/D4E4S16P8/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/57194/comment/2b3ef377_e17f6717 PS1, Line 35: device lapic 0xbeef on end # NOTE: correct Local APIC ID is
Not needed anymore, see also: […]
Yes, I will correct this along with these changes (https://review.coreboot.org/c/coreboot/+/57152).
File src/mainboard/teleplatforms/D4E4S16P8/gpio.h:
https://review.coreboot.org/c/coreboot/+/57194/comment/f643744d_b75f7483 PS13, Line 441: GpioIntDefault, GpioResetDefault, GpioTermWpu20K /*GpioTermDefault*/, GpioLockDefault} },
line over 96 characters
Please fix.
File src/mainboard/teleplatforms/D4E4S16P8/hsio.h:
https://review.coreboot.org/c/coreboot/+/57194/comment/e0970a4f_5c929e0b PS13, Line 21: * Lane[01]-> PCIe Slot |
code indent should use tabs where possible
Please fix.
File src/mainboard/teleplatforms/D4E4S16P8/hsio.h:
https://review.coreboot.org/c/coreboot/+/57194/comment/9b842987_7afa287a PS1, Line 16: * PCIE cluster #1: x2x2x2x2
trailing whitespace
Please fix.
File src/mainboard/teleplatforms/D4E4S16P8/hsio.c:
https://review.coreboot.org/c/coreboot/+/57194/comment/20a1d5f1_8402b384 PS13, Line 9: uint8_t boardid = board_id();
please, no spaces at the start of a line
Please fix.
https://review.coreboot.org/c/coreboot/+/57194/comment/3d9564ce_1671a94b PS13, Line 10: size_t num;
please, no spaces at the start of a line
Please fix.
https://review.coreboot.org/c/coreboot/+/57194/comment/3c3cc3b0_e6c93fde PS13, Line 11: switch (boardid) {
please, no spaces at the start of a line
Please fix.
https://review.coreboot.org/c/coreboot/+/57194/comment/19ff44dd_29409e96 PS13, Line 12: case BoardId_D4E4S16P8:
please, no spaces at the start of a line
Please fix.
https://review.coreboot.org/c/coreboot/+/57194/comment/d6295791_c5afcf6f PS13, Line 13: num = ARRAY_SIZE(D4E4S16P8_hsio_config);
code indent should use tabs where possible
Please fix.
https://review.coreboot.org/c/coreboot/+/57194/comment/69b7721a_52449a8d PS13, Line 13: num = ARRAY_SIZE(D4E4S16P8_hsio_config);
please, no spaces at the start of a line
Please fix.
https://review.coreboot.org/c/coreboot/+/57194/comment/f0efa3ce_f64e7fe2 PS13, Line 14: (*p_hsio_config) = (BL_HSIO_INFORMATION *)D4E4S16P8_hsio_config;
code indent should use tabs where possible
Please fix.
https://review.coreboot.org/c/coreboot/+/57194/comment/bb38dfb7_eeec809e PS13, Line 14: (*p_hsio_config) = (BL_HSIO_INFORMATION *)D4E4S16P8_hsio_config;
please, no spaces at the start of a line
Please fix.
https://review.coreboot.org/c/coreboot/+/57194/comment/c57c87da_c3188429 PS13, Line 15: break;
code indent should use tabs where possible
Please fix.
https://review.coreboot.org/c/coreboot/+/57194/comment/e41997e2_512f6d0f PS13, Line 15: break;
please, no spaces at the start of a line
Please fix.
https://review.coreboot.org/c/coreboot/+/57194/comment/117cd295_63ca83eb PS13, Line 17: num = 0;
code indent should use tabs where possible
Please fix.
https://review.coreboot.org/c/coreboot/+/57194/comment/136a61bb_9f48555e PS13, Line 17: num = 0;
please, no spaces at the start of a line
Please fix.
https://review.coreboot.org/c/coreboot/+/57194/comment/cfdd26c2_8292bdcb PS13, Line 18: (*p_hsio_config) = NULL;
code indent should use tabs where possible
Please fix.
https://review.coreboot.org/c/coreboot/+/57194/comment/82a79778_3dabff2c PS13, Line 18: (*p_hsio_config) = NULL;
please, no spaces at the start of a line
Please fix.
https://review.coreboot.org/c/coreboot/+/57194/comment/03036ce4_c22007a1 PS13, Line 19: break;
please, no spaces at the start of a line
Please fix.
https://review.coreboot.org/c/coreboot/+/57194/comment/14920a4b_82ce5626 PS13, Line 19: break;
code indent should use tabs where possible
Please fix.
https://review.coreboot.org/c/coreboot/+/57194/comment/98fd4726_90564070 PS13, Line 20: }
please, no spaces at the start of a line
Please fix.
https://review.coreboot.org/c/coreboot/+/57194/comment/5e17f32d_c8f9b91e PS13, Line 21: return num;
please, no spaces at the start of a line
Please fix.
File src/mainboard/teleplatforms/D4E4S16P8/hsio.c:
https://review.coreboot.org/c/coreboot/+/57194/comment/f16984a7_cac66144 PS15, Line 11: switch (boardid) {
switch and case should be at the same indent
Please fix.
File src/mainboard/teleplatforms/D4E4S16P8/ramstage.c:
https://review.coreboot.org/c/coreboot/+/57194/comment/09fa8f1f_f8e8f48e PS13, Line 31: memset(serial, 0, sizeof (serial));
space prohibited between function name and open parenthesis '('
Please fix.
https://review.coreboot.org/c/coreboot/+/57194/comment/ceb92a94_c1489c03 PS13, Line 32: memcpy(serial, ERROR_STRING, sizeof (ERROR_STRING));
space prohibited between function name and open parenthesis '('
Please fix.
File src/mainboard/teleplatforms/D4E4S16P8/ramstage.c:
https://review.coreboot.org/c/coreboot/+/57194/comment/fd238bbe_16af61b0 PS15, Line 31: memset (serial, 0, sizeof (serial));
space prohibited between function name and open parenthesis '('
Please fix.
https://review.coreboot.org/c/coreboot/+/57194/comment/84bbea22_eb854d9d PS15, Line 31: memset (serial, 0, sizeof (serial));
space prohibited between function name and open parenthesis '('
Please fix.
https://review.coreboot.org/c/coreboot/+/57194/comment/b930d9a7_17c572e5 PS15, Line 32: memcpy (serial, ERROR_STRING, sizeof (ERROR_STRING));
space prohibited between function name and open parenthesis '('
Please fix.
https://review.coreboot.org/c/coreboot/+/57194/comment/d05b5478_6d535899 PS15, Line 32: memcpy (serial, ERROR_STRING, sizeof (ERROR_STRING));
space prohibited between function name and open parenthesis '('
Please fix.
File src/mainboard/teleplatforms/D4E4S16P8/romstage.c:
https://review.coreboot.org/c/coreboot/+/57194/comment/928dd8d7_6d8a1656 PS15, Line 80: case: BoardId_D4E4S16P8:
labels should not be indented
Please fix.
File src/soc/intel/denverton_ns/cpu.c:
https://review.coreboot.org/c/coreboot/+/57194/comment/4c72a5fe_db3c3925 PS1, Line 258: setup_lapic();
If we don't call setup_lapic() do we have any side effect? I mean, why this is needed?
Without this change, our board hangs at the SeaBIOS stage.(https://mail.coreboot.org/hyperkitty/list/seabios@seabios.org/message/FMYHJW... ) I have checked it now. I can't give a more detailed log now - in this version of the coreboot 4.14, the output to the console is broken(it stops at some point).
Probably need to add a binding to a vendor like this? #if CONFIG(VENDOR_TELEPLATFORMS) setup_lapic(); #endif