[coreboot-gerrit] [XL] Change in coreboot[main]: mb/google/brox: Create new Brox baseboard

[Shelley Chen (Code Review)]

Attention is currently required from: Nick Vaccaro, Paul Menzel.

Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/78009?usp=email )

Change subject: mb/google/brox: Create new Brox baseboard
......................................................................

Patch Set 12:

(19 comments)

File src/mainboard/google/brox/Kconfig:

https://review.coreboot.org/c/coreboot/+/78009/comment/da496b50_d7551ca1 :
PS11, Line 145: config INCLUDE_NVIDIA_GPU_ASL
              : 	def_bool n
              : 	help
              : 	  Select this if the variant has an Nvidia GN20 GPU attached to PEG1
> Is Nvidia GN20 GPU attached to PEG1 an option for Brox?

Done

File src/mainboard/google/brox/Kconfig.name:

https://review.coreboot.org/c/coreboot/+/78009/comment/167aa5a9_04200fb8 :
PS11, Line 5: 	select BOARD_GOOGLE_BASEBOARD_BROX
> selects now go in Kconfig

I checked rex and brya and they are both formatted like this.

This config is also defined in Kconfig...would you just move it to the top of the file?  But that wouldn't work either since we need to accomodate variants right?

File src/mainboard/google/brox/acpi/power.asl:

https://review.coreboot.org/c/coreboot/+/78009/comment/0fd31b35_299fa934 :
PS11, Line 9: if CONFIG(BOARD_GOOGLE_AGAH)
            : #define GPIO_1V8_PWR_EN		GPP_F12
            : 
            : #define GPIO_NV33_PWR_EN	GPP_A21
            : #define GPIO_NV33_PG		GPP_A22
            : #else
            :
> remove

Done

https://review.coreboot.org/c/coreboot/+/78009/comment/04f139e2_89989191 :
PS11, Line 35:
> NIT - extra tab

Done

https://review.coreboot.org/c/coreboot/+/78009/comment/33ea8f15_11f4706b :
PS11, Line 47: For board revs 3 and later, two pins moved:
> This comment is not really valid as written.

Removed

https://review.coreboot.org/c/coreboot/+/78009/comment/772b0c77_69333334 :
PS11, Line 132: #if CONFIG(BOARD_GOOGLE_AGAH)
              : 	STXS (GPIO_FBVDD_PWR_EN)
              : #else
              :
> remove

Done

https://review.coreboot.org/c/coreboot/+/78009/comment/50468a46_a0710a42 :
PS11, Line 140: #if CONFIG(BOARD_GOOGLE_AGAH)
              : 	Sleep (10)
              : #else
              :
> remove

Done

https://review.coreboot.org/c/coreboot/+/78009/comment/f3fff35c_8410b33f :
PS11, Line 182: #if CONFIG(BOARD_GOOGLE_AGAH)
              : 	CTXS (GPIO_FBVDD_PWR_EN)
              : #else
              :
> remove

Done

https://review.coreboot.org/c/coreboot/+/78009/comment/cda5776a_aa514c86 :
PS11, Line 251: #if CONFIG(BOARD_GOOGLE_AGAH)
              : 	CTXS (GPIO_FBVDD_PWR_EN)
              : #else
              :
> remove

Done

https://review.coreboot.org/c/coreboot/+/78009/comment/b67ba5af_576050d5 :
PS11, Line 282: #if CONFIG(BOARD_GOOGLE_AGAH)
              : 	STXS (GPIO_FBVDD_PWR_EN)
              : #else
              :
> remove

Done

https://review.coreboot.org/c/coreboot/+/78009/comment/8b698234_9ef9e5ee :
PS11, Line 290: #if CONFIG(BOARD_GOOGLE_AGAH)
              : 	Sleep (10)
              : #else
              :
> remove

Done

File src/mainboard/google/brox/chromeos.fmd:

https://review.coreboot.org/c/coreboot/+/78009/comment/8d519d22_d10abb1e :
PS11, Line 30: brya
> brox

Done

https://review.coreboot.org/c/coreboot/+/78009/comment/c63ae177_6ae33f29 :
PS11, Line 39: ADL
> Still relevant for RPL?

So....this seems to b what brask is using?  So I think that it's still relevant for RPL?

File src/mainboard/google/brox/dsdt.asl:

https://review.coreboot.org/c/coreboot/+/78009/comment/b522d803_6505e795 :
PS11, Line 18: 	#include <cpu/intel/common/acpi/cpu.asl>
> NIT - alphabetize

Done

File src/mainboard/google/brox/variants/baseboard/brox/devicetree.cb:

https://review.coreboot.org/c/coreboot/+/78009/comment/8faa5167_54fe4f94 :
PS11, Line 76: disable_c1_state_auto_demotion
> Is this needed for RPL-UR?

I don't know so I deleted it :).

File src/mainboard/google/brox/variants/baseboard/brox/include/baseboard/ec.h:

https://review.coreboot.org/c/coreboot/+/78009/comment/fe6584eb_bab64021 :
PS11, Line 8: #include <baseboard/gpio.h>
> NIT - alphabetize

Done

https://review.coreboot.org/c/coreboot/+/78009/comment/27558f06_f5f3cea7 :
PS11, Line 10: INCLUDE_NVIDIA_GPU_ASL
> Will this be an option for Brox?

Removed.

File src/mainboard/google/brox/variants/baseboard/brox/ramstage.c:

https://review.coreboot.org/c/coreboot/+/78009/comment/836811b6_1110433f :
PS11, Line 8: 
            : #include <drivers/intel/dptf/chip.h>
            : #include <intelblocks/power_limit.h>
> NIT - alphabetize into system includes block above

Done

File src/mainboard/google/brox/variants/brox/fw_config.c:

https://review.coreboot.org/c/coreboot/+/78009/comment/a629002e_9bedfd90 :
PS11, Line 4: #include <console/console.h>
            : #include <fw_config.h>
            : #include <gpio.h>
> These aren't needed, remove.

Done