Jonathan Kollasch has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38346 )
Change subject: mainboard: add Supermicro X9SCL/X9SCM ......................................................................
Patch Set 4:
(10 comments)
https://review.coreboot.org/c/coreboot/+/38346/2/src/mainboard/supermicro/x9... File src/mainboard/supermicro/x9scl/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/38346/2/src/mainboard/supermicro/x9... PS2, Line 2: 0x0
0
Done
https://review.coreboot.org/c/coreboot/+/38346/2/src/mainboard/supermicro/x9... PS2, Line 11: end
You should "pick up" these lone `end` (put them on the previous line)
Done
https://review.coreboot.org/c/coreboot/+/38346/2/src/mainboard/supermicro/x9... PS2, Line 16: 0x0
0
Done
https://review.coreboot.org/c/coreboot/+/38346/2/src/mainboard/supermicro/x9... PS2, Line 19: end
Same here, but also make sure the various `end` are aligned. […]
Done
https://review.coreboot.org/c/coreboot/+/38346/2/src/mainboard/supermicro/x9... PS2, Line 30: register "docking_supported" = "0"
Can be dropped
Done
https://review.coreboot.org/c/coreboot/+/38346/2/src/mainboard/supermicro/x9... PS2, Line 35: register "pcie_hotplug_map" = "{ 0, 0, 0, 0, 0, 0, 0, 0 }"
Can be dropped
Done
https://review.coreboot.org/c/coreboot/+/38346/2/src/mainboard/supermicro/x9... File src/mainboard/supermicro/x9scl/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/38346/2/src/mainboard/supermicro/x9... PS2, Line 33: Scope (_SB) : { : Device (PCI0)
You can collapse this: […]
Done
https://review.coreboot.org/c/coreboot/+/38346/2/src/mainboard/supermicro/x9... File src/mainboard/supermicro/x9scl/early_init.c:
https://review.coreboot.org/c/coreboot/+/38346/2/src/mainboard/supermicro/x9... PS2, Line 45: /* 0:1d.0? */
As written on the devicetree comments, EHCI 1 is 1d.0 and EHCI2 is 1a. […]
Yes, I saw that. It's not quite clear (I admittedly haven't looked at PCH documentation) which of these 14 array members correspond to which of the USB balls on the PCH. I know where most ports on the integrated rate matching hub associated with each EHCI go, but I don't know exactly which array member those correspond to. These were done by autoport though, so I should probably just drop the un-validated comments.
https://review.coreboot.org/c/coreboot/+/38346/2/src/mainboard/supermicro/x9... PS2, Line 63: static const u16 superio_initvals[] = {
Why, just why?
Mostly to assure myself that I've matched the configuration to the OEM firmware closely, so as to have fewer things in as much question when debugging the ME issue.
https://review.coreboot.org/c/coreboot/+/38346/2/src/mainboard/supermicro/x9... File src/mainboard/supermicro/x9scl/superio.h:
PS2:
I'm pretty sure you can use things in src/superio/ instead.
Ack