[coreboot-gerrit] [L] Change in coreboot[main]: mb/intel/adlrvp: Add ADL-S DDR5 UDIMM 1DPC

[Angel Pons (Code Review)]

Attention is currently required from: Felix Singer, Martin L Roth, Maximilian Brune, Paul Menzel, Subrata Banik, Tim Wawrzynczak.

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/66545?usp=email )

Change subject: mb/intel/adlrvp: Add ADL-S DDR5 UDIMM 1DPC
......................................................................

Patch Set 23:

(4 comments)

File src/mainboard/intel/adlrvp/devicetree_s.cb:

https://review.coreboot.org/c/coreboot/+/66545/comment/bbfa0d0d_4725fb91 :
PS22, Line 2:
> The settings below apply to all ADL-S boards?

It's very unlikely that the default state of the devices in the ADL-S chipset devicetree is the desired one for this board. If you have time, at least enable/disable the devices that don't match `lspci` with vendor firmware.

In general, it seems that the intent is for ADL-S boards to follow the same structure as the other ADL-{M,N,P} boards. However, when there's only one variant, the distinction between devicetree and overridetree is impossible to define. The distinction will only start to become visible when someone else tries to add another ADL-S board: move conflicting stuff to overridetrees, keep common stuff in devicetrees.

TL;DR it looks odd, but it doesn't seem to be a big deal

File src/mainboard/intel/adlrvp/devicetree_s.cb:

https://review.coreboot.org/c/coreboot/+/66545/comment/e9ac75db_d2b81829 :
PS23, Line 4: 	register "usb2_ports[6]" = "USB2_PORT_MID(OC7)"   # USB3/2 Type A port7
            : 	register "usb2_ports[9]" = "USB2_PORT_MID(OC7)"   # USB3/2 Type A port10
            : 	register "usb2_ports[10]" = "USB2_PORT_MID(OC0)"  # USB3/2 Type A port11
            : 	r
> Add the XHCI device in the devicetree and move them into its scope.

+1

https://review.coreboot.org/c/coreboot/+/66545/comment/6cf0a1e8_8ac1a1a8 :
PS23, Line 9: # DDI_PORT_A: Combo PHY A # ADL-S RVP UDIMM 1DPC eDP1.4 Connector
            : 	# DDI_PORT_1: Combo PHY B # ADL-S RVP UDIMM 1DPC HDMI 1.4b CRLS
            : 	# DDI_PORT_2: Combo PHY C # ADL-S RVP UDIMM 1DPC DP1.4a Connector
            : 	# DDI_PORT_3: Combo PHY D # ADL-S RVP UDIMM 1DPC HDMI 2.0b ALS
            : 	# DDI_PORT_4: Combo PHY E # ADL-S RVP UDIMM 1DPC DP1.4a Connector
            : 	# Enable eDP in PortA #TODO test
            : 	#register "ddi_portA_config" = "1"
            : 	#[DDI_PORT_A] = DDI_ENABLE_HPD
            : 	register "ddi_ports_config" = "{
            : 		[DDI_PORT_1] = DDI_ENABLE_HPD,
            : 		[DDI_PORT_2] = DDI_ENABLE_HPD | DDI_ENABLE_DDC,
            : 		[DDI_PORT_3] = DDI_ENABLE_HPD,
            : 		[DDI_PORT_4] = DDI_ENABLE_HPD | DDI_ENABLE_DDC,
            : 	}"
> Add the iGPU device in the devicetree and move them into its scope.

+1

https://review.coreboot.org/c/coreboot/+/66545/comment/7471af8a_4cc842cd :
PS23, Line 24: register "sata_salp_support" = "1"
             : 	register "sata_ports_enable" = "{
             : 		[4] = 1,
             : 		[5] = 1,
             : 		[6] = 1,
             : 		[7] = 1,
             : 	}"
             : 
             : 	register "sata_ports_dev_slp" = "{
             : 		[4] = 0,
             : 		[5] = 0,
             : 		[6] = 1,
             : 		[7] = 1,
             : 	}"
> Add the SATA device in the devicetree and move them into its scope.

+1