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.