Attention is currently required from: Dinesh Gehlot, Eric Lai, Kapil Porwal, Nick Vaccaro, V Sowmya.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81262?usp=email )
Change subject: mb/google/brya: Create a tivviks variant ......................................................................
Patch Set 1:
(4 comments)
Patchset:
PS1: I am slightly perplexed.
1. You have created a new variant named `Tivviks`. I expect that each new variant will have its own variant directory.
2. You then state that Tivviks will reuse the existing Nivviks variant as is.
3. This implies that Tivviks is an updated version of Nivviks that uses the TWL SoC. I would classify this as a SKU reference. Therefore, the approach in #2 defeats the purpose of #1. If the existing Nivviks variant directory can be reused for Tivviks, then why do we even need to create a new variant named Tivviks?
4. Ideally, we should have simply ensured that Nivviks is subscribed to the TWL SoC over ADL-N. We should also have updated the backend blobs to support the TWL SoC while maintaining backward compatibility with the ADL-N SoC.
5. If the reason for creating Tivviks as a variant is to maintain a separate .config file that can be used to select TWL-specific blobs over ADL-N blobs (because converged firmware between ADL-N and TWL is not yet available), then I believe this approach makes sense. However, we will eventually have to retire Tivviks when Intel releases the converged firmware for ADL-N and TWL.
File src/mainboard/google/brya/Kconfig:
https://review.coreboot.org/c/coreboot/+/81262/comment/7bbe200d_f6b3d7e7 : PS1, Line 463: select SOC_INTEL_TWINLAKE please follow the alphabetic order
https://review.coreboot.org/c/coreboot/+/81262/comment/e3487602_43f54733 : PS1, Line 581: BOARD_GOOGLE_TIVVIKS please make a separate entry
https://review.coreboot.org/c/coreboot/+/81262/comment/039a4b84_3f7d2e0a : PS1, Line 665: BOARD_GOOGLE_TIVVIKS why not use `Tivviks` as MAINBOARD_PART_NUMBER ?
-
Subrata Banik (Code Review)