Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43087 )
Change subject: Added GBYT4 port ......................................................................
Patch Set 1:
(15 comments)
https://review.coreboot.org/c/coreboot/+/43087/1/src/mainboard/unk/Kconfig File src/mainboard/unk/Kconfig:
https://review.coreboot.org/c/coreboot/+/43087/1/src/mainboard/unk/Kconfig@1 PS1, Line 1: UNK I'd use "unknown" in symbol and folder names, it's clearer
https://review.coreboot.org/c/coreboot/+/43087/1/src/mainboard/unk/gbyt4/acp... File src/mainboard/unk/gbyt4/acpi/mainboard.asl:
https://review.coreboot.org/c/coreboot/+/43087/1/src/mainboard/unk/gbyt4/acp... PS1, Line 3: Scope (_SB.PCI0.LPEA) This is for LPE audio, if the board doesn't have any audio support, then I'd drop it
https://review.coreboot.org/c/coreboot/+/43087/1/src/mainboard/unk/gbyt4/cmo... File src/mainboard/unk/gbyt4/cmos.layout:
https://review.coreboot.org/c/coreboot/+/43087/1/src/mainboard/unk/gbyt4/cmo... PS1, Line 12: #96 4 r 0 status_c_rsvd You can drop all commented-out entries. They can only bitrot 😄
The most offensive ones are the "unused" and the last one that says "amd_reserved"
https://review.coreboot.org/c/coreboot/+/43087/1/src/mainboard/unk/gbyt4/cmo... PS1, Line 55: # SandyBridge MRC Scrambler Seed values : 896 32 r 0 mrc_scrambler_seed : 928 32 r 0 mrc_scrambler_seed_s3 Not Sandy Bridge
https://review.coreboot.org/c/coreboot/+/43087/1/src/mainboard/unk/gbyt4/dev... File src/mainboard/unk/gbyt4/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/43087/1/src/mainboard/unk/gbyt4/dev... PS1, Line 12: 0x1 0x3 is the mask for 2 ports
https://review.coreboot.org/c/coreboot/+/43087/1/src/mainboard/unk/gbyt4/dev... PS1, Line 69: off on
It's the SPI controller where the flash chip sits, AFAIK
https://review.coreboot.org/c/coreboot/+/43087/1/src/mainboard/unk/gbyt4/dev... PS1, Line 86: space
https://review.coreboot.org/c/coreboot/+/43087/1/src/mainboard/unk/gbyt4/dsd... File src/mainboard/unk/gbyt4/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/43087/1/src/mainboard/unk/gbyt4/dsd... PS1, Line 22: Scope (_SB) { : Device (PCI0) : { Ugh, that inconsistent brace placement... After removing unneeded lines, you can have a single set of braces:
Device (_SB.PCI0) { #include <soc/intel/baytrail/acpi/southcluster.asl> }
https://review.coreboot.org/c/coreboot/+/43087/1/src/mainboard/unk/gbyt4/dsd... PS1, Line 25: //#include <soc/intel/baytrail/acpi/northcluster.asl> You can drop this commented-out include
https://review.coreboot.org/c/coreboot/+/43087/1/src/mainboard/unk/gbyt4/dsd... PS1, Line 27: #include <drivers/intel/gma/acpi/default_brightness_levels.asl> If your board doesn't have an integrated LCD, this can be removed
https://review.coreboot.org/c/coreboot/+/43087/1/src/mainboard/unk/gbyt4/dsd... PS1, Line 31: #include "acpi/dptf.asl" Since DPTF is not enabled, you can drop this and remove the file
https://review.coreboot.org/c/coreboot/+/43087/1/src/mainboard/unk/gbyt4/dsd... PS1, Line 35: #include "acpi/mainboard.asl" If this file is empty, you can remove this line
https://review.coreboot.org/c/coreboot/+/43087/1/src/mainboard/unk/gbyt4/ear... File src/mainboard/unk/gbyt4/early_init.c:
PS1: I am not sure if this code is being called... To be sure, you can move this to romstage.c, more or less like I did on the Asrock Q1900M:
https://review.coreboot.org/c/coreboot/+/39658/4/src/mainboard/asrock/q1900m...
https://review.coreboot.org/c/coreboot/+/43087/1/src/mainboard/unk/gbyt4/gpi... File src/mainboard/unk/gbyt4/gpio.c:
https://review.coreboot.org/c/coreboot/+/43087/1/src/mainboard/unk/gbyt4/gpi... PS1, Line 208: struct soc_gpio_config* mainboard_get_gpios(void)
"foo* bar" should be "foo *bar"
Would be good to fix
https://review.coreboot.org/c/coreboot/+/43087/1/src/mainboard/unk/gbyt4/rom... File src/mainboard/unk/gbyt4/romstage.c:
https://review.coreboot.org/c/coreboot/+/43087/1/src/mainboard/unk/gbyt4/rom... PS1, Line 9: //static void *get_spd_pointer(char *spd_file_content, int total_spds, int *dual) You know you can comment things using preprocessor, right? 😄
#if 0 /* Tons of spaghetti code */ #endif