Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45658 ) Change subject: Port of Kontron bSC2 COMe module ...................................................................... Patch Set 3: (34 comments) https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... File src/mainboard/kontron/bSC2/Kconfig: https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... PS3, Line 19: DRIVERS_PS2_KEYBOARD instead of force-enabling this, I'd make it default to yes. https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... PS3, Line 22: SMMSTORE instead of force-enabling this, I'd make it default to yes. https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... PS3, Line 31: bCS2 bSC2? https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... PS3, Line 33: config DRAM_RESET_GATE_GPIO # FIXME: check this : int : default 60 this can be tested by verifying S3 suspend/resume capabilities. https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... PS3, Line 49: config CPU_UCODE_BINARIES : string : default "3rdparty/blobs/mainboard/$(MAINBOARDDIR)/06-2a-07" This shouldn't be needed. https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... PS3, Line 53: config DRIVERS_UART_8250IO : bool : default y the SMSC superio driver should take care of selecting this https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... File src/mainboard/kontron/bSC2/Kconfig.name: https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... PS3, Line 2: bCS2 bSC2? https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... File src/mainboard/kontron/bSC2/Makefile.inc: https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... PS3, Line 2: bootblock-y += gpio.c GPIOs aren't used in bootblock. https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... File src/mainboard/kontron/bSC2/acpi_tables.c: https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... PS3, Line 10: /* The lid is open by default. */ : gnvs->lids = 1; This only makes sense on laptops. Is this board used in a laptop? https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... File src/mainboard/kontron/bSC2/cmos.layout: PS3: Please use a SPDX header https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... PS3, Line 19: # ----------------------------------------------------------------- : # Status Register A : # ----------------------------------------------------------------- : # Status Register B : # ----------------------------------------------------------------- : # Status Register C : #96 4 r 0 status_c_rsvd : #100 1 r 0 uf_flag : #101 1 r 0 af_flag : #102 1 r 0 pf_flag : #103 1 r 0 irqf_flag : # ----------------------------------------------------------------- : # Status Register D : #104 7 r 0 status_d_rsvd : #111 1 r 0 valid_cmos_ram : # ----------------------------------------------------------------- : # Diagnostic Status Register : #112 8 r 0 diag_rsvd1 This can be removed (it's all commented out) https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... PS3, Line 40: #120 264 r 0 unused This line too https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... PS3, Line 50: unused All commented-out `unused` fields can be removed https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... PS3, Line 82: amd_reserved this isn't AMD, so this can go away too https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... File src/mainboard/kontron/bSC2/devicetree.cb: https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... PS3, Line 3: register "gfx" = "GMA_STATIC_DISPLAYS(1)" : register "gpu_cpu_backlight" = "0x00000000" : register "gpu_dp_b_hotplug" = "4" : register "gpu_dp_c_hotplug" = "4" : register "gpu_dp_d_hotplug" = "4" : register "gpu_panel_port_select" = "0" : register "gpu_panel_power_backlight_off_delay" = "2000" : register "gpu_panel_power_backlight_on_delay" = "2000" : register "gpu_panel_power_cycle_delay" = "4" : register "gpu_panel_power_down_delay" = "500" : register "gpu_panel_power_up_delay" = "100" : register "gpu_pch_backlight" = "0x13121312" Does your device have an integrated LCD? If not, this whole section can be removed https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... PS3, Line 32: register "docking_supported" = "0" Devicetree settings default to zero, this can be dropped. https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... PS3, Line 37: register "pcie_hotplug_map" = "{ 0, 0, 0, 0, 0, 0, 0, 0 }" Devicetree settings default to zero, this can be dropped. https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... PS3, Line 112: subsystemid 0x8086 0x1c09 please drop https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... PS3, Line 112: SATA Controller 2 this SATA controller corresponds to SATA ports 4 and 5 when operating in legacy IDE mode https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... PS3, Line 116: device pci 00.0 on # Host bridge : subsystemid 0x8086 0x0104 : end : device pci 01.0 on # PEG : subsystemid 0x8086 0x0101 : end : device pci 02.0 on # iGPU : subsystemid 0x8086 0x2010 : end Please move these entries above the southbridge. https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... File src/mainboard/kontron/bSC2/dsdt.asl: https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... PS3, Line 3: #define BRIGHTNESS_UP \_SB.PCI0.GFX0.INCB : #define BRIGHTNESS_DOWN \_SB.PCI0.GFX0.DECB This should not be necessary. https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... PS3, Line 27: #include <drivers/intel/gma/acpi/default_brightness_levels.asl> If your device doesn't have an integrated panel, this can be removed. https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... File src/mainboard/kontron/bSC2/early_init.c: https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... PS3, Line 33: pci_write_config16(PCI_DEV(0, 0x1f, 0), 0x82, 0x1403); /* COMA, COMB, KBC and LPC 0x2E enabled */ : pci_write_config16(PCI_DEV(0, 0x1f, 0), 0x80, 0x0010); /* LPC decode ranges COMA=3F8, COMB=2F8 */ I think these are settings are already programmed in southbridge code. https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... PS3, Line 39: smscsuperio_enable_serial(SERIAL_DEV, CONFIG_TTYS0_BASE); This should happen in `bootblock_mainboard_early_init` https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... File src/mainboard/kontron/bSC2/hda_verb.c: https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... PS3, Line 1: ## SPDX-License-Identifier: GPL-2.0-only
spaces required around that ':' (ctx:VxW) Um, this is not the comment type you want to use here.
https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... PS3, Line 9: 0x00000004 4 https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... PS3, Line 10: /* NID 0x01: Subsystem ID. */ These comments don't add any value. I'd just drop them. https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... PS3, Line 11: 0x3 3 https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... PS3, Line 14: 0x3 3 https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... PS3, Line 17: 0x3 3 https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... PS3, Line 20: 0x3 3 https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... File src/mainboard/kontron/bSC2/mainboard.c: https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... PS3, Line 1: ## SPDX-License-Identifier: GPL-2.0-only
spaces required around that ':' (ctx:VxW) Wrong comment type
https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... PS3, Line 103: if ((vendid = 0x41) && // Analog Devices
do not use assignment in if condition this is a bug.
https://review.coreboot.org/c/coreboot/+/45658/3/src/mainboard/kontron/bSC2/... PS3, Line 106: ADT7490 Don't we have a driver for this? I think we do. If not, this should be added as a driver. -- To view, visit https://review.coreboot.org/c/coreboot/+/45658 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2932af6016b044fa807da00acc7b147ef1c7c45e Gerrit-Change-Number: 45658 Gerrit-PatchSet: 3 Gerrit-Owner: Jose Trujillo Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Thu, 24 Sep 2020 00:05:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: comment