Change in coreboot[master]: mb/supermicro/x9scl: Split IPMI support into a variant

[Nico Huber (Code Review)]

Hello Jonathan Kollasch, Angel Pons, Michael Niewöhner, Kyösti Mälkki,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/coreboot/+/43085

to review the following change.

Change subject: mb/supermicro/x9scl: Split IPMI support into a variant
......................................................................

mb/supermicro/x9scl: Split IPMI support into a variant

The devicetree already mentioned the device, but without the driver
selected that didn't do anything. Selecting the driver for all variants
isn't an option as that would result in incredibly long timeouts on
systems without the hardware.

Change-Id: I7cc348135469052c595297226e5e86c040de4746
Signed-off-by: Nico Huber 
---
M src/mainboard/supermicro/x9scl/Kconfig
M src/mainboard/supermicro/x9scl/Kconfig.name
M src/mainboard/supermicro/x9scl/devicetree.cb
A src/mainboard/supermicro/x9scl/variants/x9scl-f/overridetree.cb
4 files changed, 23 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/43085/1

diff --git a/src/mainboard/supermicro/x9scl/Kconfig b/src/mainboard/supermicro/x9scl/Kconfig
index fb73683..22a8702 100644
--- a/src/mainboard/supermicro/x9scl/Kconfig
+++ b/src/mainboard/supermicro/x9scl/Kconfig
@@ -1,4 +1,4 @@
-if BOARD_SUPERMICRO_X9SCL
+if BOARD_SUPERMICRO_X9SCL || BOARD_SUPERMICRO_X9SCL_F

config BOARD_SPECIFIC_OPTIONS
    def_bool y
@@ -22,6 +22,10 @@
    string
    default "X9SCL/X9SCM"

+config OVERRIDE_DEVICETREE
+	string
+	default "variants/x9scl-f/overridetree.cb" if BOARD_SUPERMICRO_X9SCL_F
+
 config USBDEBUG_HCD_INDEX
    int
    default 1
diff --git a/src/mainboard/supermicro/x9scl/Kconfig.name b/src/mainboard/supermicro/x9scl/Kconfig.name
index e0e91f1..2a3cd13 100644
--- a/src/mainboard/supermicro/x9scl/Kconfig.name
+++ b/src/mainboard/supermicro/x9scl/Kconfig.name
@@ -1,2 +1,6 @@
 config BOARD_SUPERMICRO_X9SCL
    bool "X9SCL/X9SCM"
+
+config BOARD_SUPERMICRO_X9SCL_F
+	bool "X9SCL-F/X9SCM-F (w/ BMC)"
+	select IPMI_KCS
diff --git a/src/mainboard/supermicro/x9scl/devicetree.cb b/src/mainboard/supermicro/x9scl/devicetree.cb
index 9236f6f..3827379 100644
--- a/src/mainboard/supermicro/x9scl/devicetree.cb
+++ b/src/mainboard/supermicro/x9scl/devicetree.cb
@@ -22,7 +22,6 @@
    		register "c2_latency" = "0x0065"
    		register "gen1_dec" = "0x00fc0a01" # NCT6776 SuperIO (0x0a00-0aff)
    		register "gen2_dec" = "0x00fc1641" # WPCM450 SuperIO (0x1600-16ff)
-			register "gen3_dec" = "0x00040ca1" # IPMI KCS (0x0ca0-0ca3)
    		register "gen4_dec" = "0x001c03e1" # 3rd UART (0x03e0-03ff)
    		register "pcie_port_coalesce" = "1"
    		register "sata_interface_speed_support" = "0x3"
@@ -104,11 +103,6 @@
    				device pnp 2e.16 off end	# Deep sleep
    				device pnp 2e.17 off end	# GPIOA
    			end
-				chip drivers/ipmi
-					register "wait_for_bmc" = "1"
-					register "bmc_boot_timeout" = "60"
-					device pnp ca2.0 off end	# IPMI KCS
-				end
    			chip superio/nuvoton/wpcm450
    				device pnp 164e.2 on
    					io 0x60 = 0x03e8
diff --git a/src/mainboard/supermicro/x9scl/variants/x9scl-f/overridetree.cb b/src/mainboard/supermicro/x9scl/variants/x9scl-f/overridetree.cb
new file mode 100644
index 0000000..9d713f1
--- /dev/null
+++ b/src/mainboard/supermicro/x9scl/variants/x9scl-f/overridetree.cb
@@ -0,0 +1,14 @@
+chip northbridge/intel/sandybridge
+	device domain 0 on
+		chip southbridge/intel/bd82x6x # Intel Series 6 Cougar Point PCH
+			register "gen3_dec" = "0x00040ca1" # IPMI KCS (0x0ca0-0ca3)
+			device pci 1f.0 on	# LPC bridge
+				chip drivers/ipmi
+					register "wait_for_bmc" = "1"
+					register "bmc_boot_timeout" = "60"
+					device pnp ca2.0 off end	# IPMI KCS
+				end
+			end
+		end
+	end
+end

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43085 )

Change subject: mb/supermicro/x9scl: Split IPMI support into a variant
......................................................................

Patch Set 1: Code-Review+2

[Jonathan Kollasch (Code Review)]

Jonathan Kollasch has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43085 )

Change subject: mb/supermicro/x9scl: Split IPMI support into a variant
......................................................................

Patch Set 1: Code-Review-1

[Jonathan Kollasch (Code Review)]

Jonathan Kollasch has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43085 )

Change subject: mb/supermicro/x9scl: Split IPMI support into a variant
......................................................................

Patch Set 1:

Unless we're going to insist that all devices in the device tree be enabled, there is no need for this; the long timeout doesn't happen when the ipmi driver is present but disabled in the device tree.

Additionally, I strongly object to different coreboot variants/builds for boards that are supported by a single platform firmware build from the original board vendor, as is the case with the X9SCL/X9SCL-F/X9SCM/X9SCM-F/X9SCL+-F/X9SCM-II-F (and X9SCL+/X9SCM-II if they ever were made).

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43085 )

Change subject: mb/supermicro/x9scl: Split IPMI support into a variant
......................................................................

Patch Set 1:
> Unless we're going to insist that all devices in the device tree be enabled, there is no need for this; the long timeout doesn't happen when the ipmi driver is present but disabled in the device tree.

Well, the current inconsistent devicetree is holding CB:35086
back for long enough. So it's either

a) variants (this),
b) unconditionally selecting IPMI_KCS (CB:41687), or
c) removing `chip drivers/ipmi` from the devicetree (for now).

Which would you prefer?
> Additionally, I strongly object to different coreboot variants/builds for boards that are supported by a single platform firmware build from the original board vendor, as is the case with the X9SCL/X9SCL-F/X9SCM/X9SCM-F/X9SCL+-F/X9SCM-II-F (and X9SCL+/X9SCM-II if they ever were made).

coreboot code often depends on compile-time decisions. If you
want to change that, just do so. But please don't make other
people wait for it.

[Nico Huber (Code Review)]

Nico Huber has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/43085 )

Change subject: mb/supermicro/x9scl: Split IPMI support into a variant
......................................................................

Abandoned

No support preferred over variants.