[coreboot-gerrit] [XL] Change in coreboot[main]: mb/intel/ptlrvp: Add Intel Panther Lake RVP as copy of google/fatcat

[Cliff Huang (Code Review)]

Attention is currently required from: Jamie Ryu, Jérémy Compostella, Paul Menzel.

Cliff Huang has posted comments on this change by Jérémy Compostella. ( https://review.coreboot.org/c/coreboot/+/84564?usp=email )

Change subject: mb/intel/ptlrvp: Add Intel Panther Lake RVP as copy of google/fatcat
......................................................................

Patch Set 6:

(9 comments)

File src/mainboard/intel/ptlrvp/Kconfig:

https://review.coreboot.org/c/coreboot/+/84564/comment/79d0141f_6e2fdb5d?usp... :
PS6, Line 2: 
forgot how we made UART changes for Windows RVP. should we have  optional flag for that?

https://review.coreboot.org/c/coreboot/+/84564/comment/2047bbe2_fce2637f?usp... :
PS6, Line 3: config BOARD_INTEL_PTLRVP_COMMON
Can we add commented flag that we used to use fill_policy instead of fsp_param back?

https://review.coreboot.org/c/coreboot/+/84564/comment/f0a87c3a_f6810be2?usp... :
PS6, Line 5: 	select BOARD_ROMSIZE_KB_32768
fatcat has CPU_INTEL_SOCKET_OTHER for SMBios changes.

https://review.coreboot.org/c/coreboot/+/84564/comment/f870ba5d_116ce06b?usp... :
PS6, Line 69: if BOARD_INTEL_PTLRVP_COMMON
do we want to remove BOARD_INTEL_PTLRVP_COMMON since we use board_id instead of variants? only keep variant for ptlrvp for structural consistency with fatcat, where ODM will add more variants?

File src/mainboard/intel/ptlrvp/chromeos.fmd:

PS6: 
quit a lot of difference compared to recent fatcat.  do we need to port those?

File src/mainboard/intel/ptlrvp/variants/baseboard/ptlrvp/include/baseboard/ec.h:

https://review.coreboot.org/c/coreboot/+/84564/comment/88baf8e0_ac35a51f?usp... :
PS6, Line 78: #define EC_ENABLE_SYNC_IRQ      /* Enable tight timestamp / wake support */
do we need these two when ISH enabled like Fatcat does?

File src/mainboard/intel/ptlrvp/variants/baseboard/ptlrvp/include/baseboard/gpio.h:

https://review.coreboot.org/c/coreboot/+/84564/comment/57515a9a_ac84e0ff?usp... :
PS6, Line 13: #define EC_SYNC_IRQ	0
should we remove these two defines? it is defined in src/mainboard/intel/ptlrvp/variants/ptlrvp/include/baseboard/gpio.h

in fact, we could just remove this header.

File src/mainboard/intel/ptlrvp/variants/ptlrvp/devicetree.cb:

PS6: 
why do we need this file?  We already have variants/baseboard/ptlrvp/devicetree.cb.

File src/mainboard/intel/ptlrvp/variants/ptlrvp/gpio.c:

https://review.coreboot.org/c/coreboot/+/84564/comment/165c0075_91fa0c08?usp... :
PS6, Line 199: 	/* GPP_E07:     Not used */
in fatcat GPIO, GPP_07 is configured for EC_SOC_INT_ODL unless ISH is enabled. please see: src/mainboard/google/fatcat/variants/fatcat/gpio.c