[coreboot-gerrit] [L] Change in coreboot[main]: [WIP] OptiPlex 3050 port

[Máté Kukri (Code Review)]

Attention is currently required from: Angel Pons, Felix Singer.

Máté Kukri has posted comments on this change by Máté Kukri. ( https://review.coreboot.org/c/coreboot/+/82053?usp=email )

Change subject: [WIP] OptiPlex 3050 port
......................................................................

Patch Set 11:

(10 comments)

File src/mainboard/dell/optiplex_3050/Kconfig:

https://review.coreboot.org/c/coreboot/+/82053/comment/70374ab2_8a61fefd?usp... :
PS11, Line 12: 	# select INTEL_GMA_HAVE_VBT
> Is this a FIXME?

Yes

https://review.coreboot.org/c/coreboot/+/82053/comment/5d213b0d_5cfaffb2?usp... :
PS11, Line 30: config PRERAM_CBMEM_CONSOLE_SIZE
             : 	hex
             : 	default 0xd00
> Is this needed? If not, I'd drop it. If it's worth keeping, I'd remove the type: […]

I suspect not, this is probably a copypasta leftover.

File src/mainboard/dell/optiplex_3050/acpi/dptf.asl:

PS11:
> Is this tested?

i think i saw a dmesg about it, but otherwise no.

File src/mainboard/dell/optiplex_3050/bootblock.c:

https://review.coreboot.org/c/coreboot/+/82053/comment/84865eb6_1d856cb5?usp... :
PS11, Line 55: 	sch5555_mbox_read(1, 0xb8);
> I wonder what these reads do

vendor fw also does the reads and similarly discard the results, outside that i have no idea

https://review.coreboot.org/c/coreboot/+/82053/comment/9c4560c7_43ee5823?usp... :
PS11, Line 96: 	// Changes LED color among a few other things
> Is the functioning of these bits known?

i think a datasheet for a similar ec discribes SCH555x_RUNTIME_PME_STS, SCH555x_RUNTIME_PME_EN and SCH555x_RUNTIME_LED.

SCH555x_RUNTIME_UNK1 is named UNK1 because it's not documented

File src/mainboard/dell/optiplex_3050/devicetree.cb:

https://review.coreboot.org/c/coreboot/+/82053/comment/d8cf0325_9fc82000?usp... :
PS11, Line 36: 	register "SendVrMbxCmd" = "2"
> Is this copy-pasta?

yes

https://review.coreboot.org/c/coreboot/+/82053/comment/72300c8b_a14579ff?usp... :
PS11, Line 76: 		# ME interface is 'off' to avoid HECI reset delay due to HAP
> Would be nice to auto-disable at runtime.

yes indeed, although the bootguard bypass is currently only tested to work in HAP mode (it should be possible to make it work otherwise in theory)

https://review.coreboot.org/c/coreboot/+/82053/comment/196ba264_2b414e8d?usp... :
PS11, Line 110: 			register "SerialIoDevMode" = "{ [PchSerialIoIndexUart0] = PchSerialIoPci, }"
>

Fix applied.

File src/mainboard/dell/optiplex_3050/include/gpio.h:

https://review.coreboot.org/c/coreboot/+/82053/comment/f0c5e1e8_e6ae2790?usp... :
PS11, Line 15: /* Pad configuration was generated automatically using intelp2m utility */
> intelp2m has some command-line args to generate pretty macros, I think you have to tell it to ignore […]

I am planning on re-writing the GPIO table based on schematic.

File src/mainboard/dell/optiplex_3050/romstage.c:

https://review.coreboot.org/c/coreboot/+/82053/comment/3c23a0b9_81b2dc2a?usp... :
PS11, Line 24: 	 * FIXME: do we need this? */
> I think this is used with HDA. I think it should be auto-configured.

I'll test if this can be removed when i add audio