View Change

34 comments:

• File src/mainboard/kontron/bSC2/Kconfig:

  • Patch Set #3, Line 19: DRIVERS_PS2_KEYBOARD

    instead of force-enabling this, I'd make it default to yes.

  • Patch Set #3, Line 22: SMMSTORE

    instead of force-enabling this, I'd make it default to yes.

  • Patch Set #3, Line 31: bCS2

    bSC2?

  • Patch Set #3, Line 33:

    config DRAM_RESET_GATE_GPIO # FIXME: check this
    	int
    	default 60
    

    this can be tested by verifying S3 suspend/resume capabilities.

  • Patch Set #3, Line 49:

    config CPU_UCODE_BINARIES
    	string
    	default "3rdparty/blobs/mainboard/$(MAINBOARDDIR)/06-2a-07"
    

    This shouldn't be needed.

  • Patch Set #3, Line 53:

    config DRIVERS_UART_8250IO
    	bool
    	default y
    

    the SMSC superio driver should take care of selecting this

• File src/mainboard/kontron/bSC2/Kconfig.name:

  • Patch Set #3, Line 2: bCS2

    bSC2?

• File src/mainboard/kontron/bSC2/Makefile.inc:

  • Patch Set #3, Line 2: bootblock-y += gpio.c

    GPIOs aren't used in bootblock.

• File src/mainboard/kontron/bSC2/acpi_tables.c:

  • Patch Set #3, Line 10:

    /* The lid is open by default. */
    	gnvs->lids = 1;
    

    This only makes sense on laptops. Is this board used in a laptop?

• File src/mainboard/kontron/bSC2/cmos.layout:

  • Patch Set #3:

    Please use a SPDX header

  • Patch Set #3, 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)

  • Patch Set #3, Line 40: #120 264 r 0 unused

    This line too

  • Patch Set #3, Line 50: unused

    All commented-out `unused` fields can be removed

  • Patch Set #3, Line 82: amd_reserved

    this isn't AMD, so this can go away too

• File src/mainboard/kontron/bSC2/devicetree.cb:

  • Patch Set #3, 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

  • Patch Set #3, Line 32: register "docking_supported" = "0"

    Devicetree settings default to zero, this can be dropped.

  • Patch Set #3, Line 37: register "pcie_hotplug_map" = "{ 0, 0, 0, 0, 0, 0, 0, 0 }"

    Devicetree settings default to zero, this can be dropped.

  • Patch Set #3, Line 112: subsystemid 0x8086 0x1c09

    please drop

  • Patch Set #3, Line 112: SATA Controller 2

    this SATA controller corresponds to SATA ports 4 and 5 when operating in legacy IDE mode

  • Patch Set #3, 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.

• File src/mainboard/kontron/bSC2/dsdt.asl:

  • Patch Set #3, Line 3:

    #define BRIGHTNESS_UP \_SB.PCI0.GFX0.INCB
    #define BRIGHTNESS_DOWN \_SB.PCI0.GFX0.DECB
    

    This should not be necessary.

  • Patch Set #3, Line 27: #include <drivers/intel/gma/acpi/default_brightness_levels.asl>

    If your device doesn't have an integrated panel, this can be removed.

• File src/mainboard/kontron/bSC2/early_init.c:

  • Patch Set #3, 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.

  • Patch Set #3, Line 39: smscsuperio_enable_serial(SERIAL_DEV, CONFIG_TTYS0_BASE);

    This should happen in `bootblock_mainboard_early_init`

• File src/mainboard/kontron/bSC2/hda_verb.c:

  • Patch Set #3, 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.

  • Patch Set #3, Line 9: 0x00000004

    4

  • Patch Set #3, Line 10: /* NID 0x01: Subsystem ID. */

    These comments don't add any value. I'd just drop them.

  • Patch Set #3, Line 11: 0x3

    3

  • Patch Set #3, Line 14: 0x3

    3

  • Patch Set #3, Line 17: 0x3

    3

  • Patch Set #3, Line 20: 0x3

    3

• File src/mainboard/kontron/bSC2/mainboard.c:

  • Patch Set #3, Line 1: ## SPDX-License-Identifier: GPL-2.0-only

    spaces required around that ':' (ctx:VxW)

    Wrong comment type

  • Patch Set #3, Line 103: if ((vendid = 0x41) && // Analog Devices

    do not use assignment in if condition

    this is a bug.

  • Patch Set #3, 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 change 45658. To unsubscribe, or for help writing mail filters, visit settings.