Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29480 )
Change subject: [WIP] mb/kontron/bsl6: Add new Skylake COMe module ......................................................................
Patch Set 11: Code-Review+1
(11 comments)
Any documentation?
https://review.coreboot.org/c/coreboot/+/29480/11/src/mainboard/kontron/bsl6... File src/mainboard/kontron/bsl6/Kconfig:
https://review.coreboot.org/c/coreboot/+/29480/11/src/mainboard/kontron/bsl6... PS11, Line 3: # dummy Please remove
https://review.coreboot.org/c/coreboot/+/29480/11/src/mainboard/kontron/bsl6... PS11, Line 15: select MAINBOARD_USES_FSP2_0 Not sure if this is needed anymore with SKL FSP 1.1 dropped
https://review.coreboot.org/c/coreboot/+/29480/11/src/mainboard/kontron/bsl6... File src/mainboard/kontron/bsl6/acpi/mainboard.asl:
https://review.coreboot.org/c/coreboot/+/29480/11/src/mainboard/kontron/bsl6... PS11, Line 19: Scope (_SB) : { : Device (PWRB) Maybe replace with "Device (_SB.PWRB)" ?
https://review.coreboot.org/c/coreboot/+/29480/11/src/mainboard/kontron/bsl6... File src/mainboard/kontron/bsl6/cmos.layout:
PS11: Are all the commented-out entries needed?
https://review.coreboot.org/c/coreboot/+/29480/11/src/mainboard/kontron/bsl6... File src/mainboard/kontron/bsl6/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/29480/11/src/mainboard/kontron/bsl6... PS11, Line 37: Scope (_SB) { : Device (PCI0) Device (_SB.PCI0)
https://review.coreboot.org/c/coreboot/+/29480/11/src/mainboard/kontron/bsl6... PS11, Line 46: #include <soc/intel/skylake/acpi/sleepstates.asl> I'd rather #include <southbridge/intel/common/acpi/sleepstates.asl>
https://review.coreboot.org/c/coreboot/+/29480/11/src/mainboard/kontron/bsl6... File src/mainboard/kontron/bsl6/gpio.h:
https://review.coreboot.org/c/coreboot/+/29480/11/src/mainboard/kontron/bsl6... PS11, Line 24: #define GPIO_SPEAKER_MAXIM_AMP_SDMODE GPP_B2 Seems copied off some Google board
https://review.coreboot.org/c/coreboot/+/29480/11/src/mainboard/kontron/bsl6... File src/mainboard/kontron/bsl6/variants/boxer26/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/29480/11/src/mainboard/kontron/bsl6... PS11, Line 104: register "i2c_voltage[4]" = "I2C_VOLTAGE_1V8" # I2C4 is 1.8V Seems copied off some Google board
https://review.coreboot.org/c/coreboot/+/29480/11/src/mainboard/kontron/bsl6... PS11, Line 135: device pci 19.1 off end # I2C #5 : device pci 19.2 off end # I2C #4 This inversion looks odd, but I didn't open any datasheet
https://review.coreboot.org/c/coreboot/+/29480/11/src/mainboard/kontron/bsl6... File src/mainboard/kontron/bsl6/variants/bsl6/devicetree.cb:
PS11: Some of that would better fit into an overridetree
https://review.coreboot.org/c/coreboot/+/29480/11/src/mainboard/kontron/bsl6... PS11, Line 120: register "i2c_voltage[4]" = "I2C_VOLTAGE_1V8" # I2C4 is 1.8V Looks copied
-
Angel Pons (Code Review)