Change in coreboot[master]: mb/purism/librem_cnl: Adjust in preparation for new variants
Matt DeVillier has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47051 )
Change subject: mb/purism/librem_cnl: Adjust in preparation for new variants ......................................................................
mb/purism/librem_cnl: Adjust in preparation for new variants
- Move the SoC select to board config (vs baseboard config) - Qualify the VGA PCI ID and CBFS size values based on board selection - Move devicetree to variant dir and add Kconfig entry - Use a separate board_info.txt for the baseboard and each variant
Change-Id: I4764f2c1243ea49bd08e0735865cc3cb7a66441f Signed-off-by: Matt DeVillier matt.devillier@puri.sm --- M src/mainboard/purism/librem_cnl/Kconfig M src/mainboard/purism/librem_cnl/Kconfig.name M src/mainboard/purism/librem_cnl/board_info.txt A src/mainboard/purism/librem_cnl/variants/librem_mini/board_info.txt R src/mainboard/purism/librem_cnl/variants/librem_mini/devicetree.cb 5 files changed, 17 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/47051/1
diff --git a/src/mainboard/purism/librem_cnl/Kconfig b/src/mainboard/purism/librem_cnl/Kconfig index 38be380..464350c 100644 --- a/src/mainboard/purism/librem_cnl/Kconfig +++ b/src/mainboard/purism/librem_cnl/Kconfig @@ -9,7 +9,6 @@ select NO_UART_ON_SUPERIO select SOC_INTEL_COMMON_BLOCK_HDA select SOC_INTEL_COMMON_BLOCK_HDA_VERB - select SOC_INTEL_WHISKEYLAKE select SPD_READ_BY_WORD select USE_LEGACY_8254_TIMER
@@ -31,9 +30,13 @@ string default "librem_mini" if BOARD_PURISM_LIBREM_MINI
+config DEVICETREE + string + default "variants/$(CONFIG_VARIANT_DIR)/devicetree.cb" + config CBFS_SIZE hex - default 0x800000 + default 0x800000 if BOARD_PURISM_LIBREM_MINI
config MAX_CPUS int @@ -49,7 +52,7 @@
config VGA_BIOS_ID string - default "8086,3ea0" + default "8086,3ea0" if BOARD_PURISM_LIBREM_MINI
config PXE_ROM_ID string diff --git a/src/mainboard/purism/librem_cnl/Kconfig.name b/src/mainboard/purism/librem_cnl/Kconfig.name index 326165ba..83f1495 100644 --- a/src/mainboard/purism/librem_cnl/Kconfig.name +++ b/src/mainboard/purism/librem_cnl/Kconfig.name @@ -1,3 +1,4 @@ config BOARD_PURISM_LIBREM_MINI bool "Librem Mini" select BOARD_PURISM_BASEBOARD_LIBREM_CNL + select SOC_INTEL_WHISKEYLAKE diff --git a/src/mainboard/purism/librem_cnl/board_info.txt b/src/mainboard/purism/librem_cnl/board_info.txt index ca61edd..6c7620c 100644 --- a/src/mainboard/purism/librem_cnl/board_info.txt +++ b/src/mainboard/purism/librem_cnl/board_info.txt @@ -1,6 +1,6 @@ Vendor name: Purism -Board name: librem_cnl -Category: desktop +Board name: Librem Cannonlake baseboard +Category: misc Release year: 2020 ROM package: SOIC-8 ROM protocol: SPI diff --git a/src/mainboard/purism/librem_cnl/variants/librem_mini/board_info.txt b/src/mainboard/purism/librem_cnl/variants/librem_mini/board_info.txt new file mode 100644 index 0000000..843ff9f --- /dev/null +++ b/src/mainboard/purism/librem_cnl/variants/librem_mini/board_info.txt @@ -0,0 +1,8 @@ +Vendor name: Purism +Board name: Librem Mini +Category: desktop +Release year: 2020 +ROM package: SOIC-8 +ROM protocol: SPI +ROM socketed: n +Flashrom support: y diff --git a/src/mainboard/purism/librem_cnl/devicetree.cb b/src/mainboard/purism/librem_cnl/variants/librem_mini/devicetree.cb similarity index 100% rename from src/mainboard/purism/librem_cnl/devicetree.cb rename to src/mainboard/purism/librem_cnl/variants/librem_mini/devicetree.cb
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47051 )
Change subject: mb/purism/librem_cnl: Adjust in preparation for new variants ......................................................................
Patch Set 1: Code-Review+2
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47051 )
Change subject: mb/purism/librem_cnl: Adjust in preparation for new variants ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... File src/mainboard/purism/librem_cnl/Kconfig:
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... PS1, Line 35: CONFIG_ nit: CONFIG_ can be omitted iirc
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... File src/mainboard/purism/librem_cnl/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... PS1, Line 4: select SOC_INTEL_WHISKEYLAKE do that in Kconfig, please; have a look at google/hatch how that can be handled nicely 😊
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47051 )
Change subject: mb/purism/librem_cnl: Adjust in preparation for new variants ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... File src/mainboard/purism/librem_cnl/Kconfig:
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... PS1, Line 35: CONFIG_
nit: CONFIG_ can be omitted iirc
x11-lga1151-series still has it, so I assume it can't be removed (which is what I'd expect)
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... File src/mainboard/purism/librem_cnl/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... PS1, Line 4: select SOC_INTEL_WHISKEYLAKE
do that in Kconfig, please; have a look at google/hatch how that can be handled nicely 😊
Hatch only has to deal with one type of SoC. I'm not sure what your point is. Personally, I prefer to have mainboards select non-common options in Kconfig.name instead of piling up conditional selects in Kconfig
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47051 )
Change subject: mb/purism/librem_cnl: Adjust in preparation for new variants ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... File src/mainboard/purism/librem_cnl/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... PS1, Line 4: select SOC_INTEL_WHISKEYLAKE
Hatch only has to deal with one type of SoC. I'm not sure what your point is.
Right, but it shows how different boards can select different options, while having a common section (BOARD_GOOGLE_HATCH_COMMON, BOARD_GOOGLE_BASEBOARD_PUFF, BOARD_GOOGLE_BASEBOARD_HATCH).
Personally, I prefer to have mainboards select non-common options in Kconfig.name instead of piling up conditional selects in Kconfig
I don't, because it's wrong. Kconfig.name ist not meant for selecting anything but shall just be used for the names in choice (as the filename already says). This is documented btw. `Documentation/getting_started/kconfig.md`. I know there are boards doing that, but it's not right.
If we want to split it to different Kconfig, the right way would be having `board/variant/*/Kconfig`, which overrides options in `board/Kconfig` per variant.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47051 )
Change subject: mb/purism/librem_cnl: Adjust in preparation for new variants ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... File src/mainboard/purism/librem_cnl/Kconfig:
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... PS1, Line 35: CONFIG_
x11-lga1151-series still has it, so I assume it can't be removed (which is what I'd expect)
oops, you're right. can't be removed because VARIANT_DIR is not declared globally yet
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47051 )
Change subject: mb/purism/librem_cnl: Adjust in preparation for new variants ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... File src/mainboard/purism/librem_cnl/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... PS1, Line 4: select SOC_INTEL_WHISKEYLAKE
Hatch only has to deal with one type of SoC. I'm not sure what your point is. […]
You mean the example here? https://doc.coreboot.org/getting_started/kconfig.html#source
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47051 )
Change subject: mb/purism/librem_cnl: Adjust in preparation for new variants ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... File src/mainboard/purism/librem_cnl/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... PS1, Line 4: select SOC_INTEL_WHISKEYLAKE
You mean the example here? https://doc.coreboot.org/getting_started/kconfig. […]
Yes, actually that: "Each mainboard’s Kconfig.name file contains just two statements that generate a list of all the platform names:"
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47051 )
Change subject: mb/purism/librem_cnl: Adjust in preparation for new variants ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... File src/mainboard/purism/librem_cnl/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... PS1, Line 4: select SOC_INTEL_WHISKEYLAKE
Yes, actually that: "Each mainboard’s Kconfig. […]
This is just describing the example, it is definitely not a rule. Of course, if the sentence is taken out of context, it could mean anything. :)
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47051 )
Change subject: mb/purism/librem_cnl: Adjust in preparation for new variants ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... File src/mainboard/purism/librem_cnl/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... PS1, Line 4: select SOC_INTEL_WHISKEYLAKE
This is just describing the example, it is definitely not a rule. […]
It doesn't look like a rule, yeah but it still makes it pretty clear how that choice/names mechanism *should* be used. What I fear is, that if we start adding stuff to Kconfig.names, is that people will follow and Kconfig/Kconfig.names get mixed up very soon, making it pretty confusing. Surely that won't happen with a single select, but I'm sure more will follow :D
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47051 )
Change subject: mb/purism/librem_cnl: Adjust in preparation for new variants ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... File src/mainboard/purism/librem_cnl/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... PS1, Line 4: select SOC_INTEL_WHISKEYLAKE
It doesn't look like a rule, yeah but it still makes it pretty clear how that choice/names mechanism […]
The Kconfig.name thing is a bit of a hack to work around deficiencies in the Kconfig language. While it would be nice to keep options close to the board's definition, I also agree that we should limit Kconfig.name's scope to filling in board names in the choice.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47051 )
Change subject: mb/purism/librem_cnl: Adjust in preparation for new variants ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... File src/mainboard/purism/librem_cnl/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... PS1, Line 4: select SOC_INTEL_WHISKEYLAKE
The Kconfig.name thing is a bit of a hack to work around deficiencies in the Kconfig language. […]
I don't understand using HATCH as a model, when the boards listed in Kconfig.name there clearly select more than just the baseboard -- they select overrides, audio drivers, etc.
Having each board select the correct SoC and having everything else in Kconfig is the cleanest way to do it; otherwise, I'm defining two SoC-specific baseboards, each of which selects the LIBREM_CNL baseboard, all just to not have the SoC selected in Kconfig.name
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47051 )
Change subject: mb/purism/librem_cnl: Adjust in preparation for new variants ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... File src/mainboard/purism/librem_cnl/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... PS1, Line 4: select SOC_INTEL_WHISKEYLAKE
I don't understand using HATCH as a model, when the boards listed in Kconfig. […]
I generally agree with Michael's sentiment. But placing selects in `Kconfig.name` is already common practice for a year or so or maybe more. If somebody wants to stop that, this is not the right place to discuss it.
If somebody feels strong about it, please discuss it on the mailing list, and add lint scripts to prevent further spreading in case.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47051 )
Change subject: mb/purism/librem_cnl: Adjust in preparation for new variants ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... File src/mainboard/purism/librem_cnl/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... PS1, Line 4: select SOC_INTEL_WHISKEYLAKE
I don't understand using HATCH as a model, when the boards listed in Kconfig.name there clearly select more than just the baseboard -- they select overrides, audio drivers, etc.
Forget hatch and have a look at google/poppy.
Having each board select the correct SoC and having everything else in Kconfig is the cleanest way to do it; otherwise, I'm defining two SoC-specific baseboards, each of which selects the LIBREM_CNL baseboard, all just to not have the SoC selected in Kconfig.name
Well, the cleanest way IMO *is* having two SoC-specific baseboards or even having another Kconfig per variant (board/variant/*/Kconfig). Are you 100% sure it will only be the SoC? What when you realize you need to override another Kconfig? Do you add that to Kconfig.name, too, then?
I generally agree with Michael's sentiment. But placing selects in `Kconfig.name` is already common practice for a year or so or maybe more. If somebody wants to stop that, this is not the right place to discuss it.
I'm not completely against having a select in Kconfig.name, even when that would be the best - selecting the baseboard and a board variant (if done right, only the latter) should be ok.
Just think about this once again, please. I won't block this, if you all agree that this needs more general discussion.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47051 )
Change subject: mb/purism/librem_cnl: Adjust in preparation for new variants ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... File src/mainboard/purism/librem_cnl/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... PS1, Line 4: select SOC_INTEL_WHISKEYLAKE
I don't understand using HATCH as a model, when the boards listed in Kconfig. […]
IMHO, this already looks quite messy already: https://github.com/coreboot/coreboot/blob/master/src/mainboard/intel/coffeel...
But guess what? We can actually try something that would make everyone happy enough:
Kconfig.name:
config BOARD_FOO_CFL_H bool "Foo Coffee Lake H board" select BOARD_FOO_CFL_SERIES_BASEBOARD
config BOARD_FOO_CFL_S bool "Foo Coffee Lake S board" select BOARD_FOO_CFL_SERIES_BASEBOARD
config BOARD_FOO_CFL_U bool "Foo Coffee Lake U board" select BOARD_FOO_CFL_SERIES_BASEBOARD
config BOARD_FOO_WHL_U bool "Foo Whiskey Lake U board" select BOARD_FOO_CFL_SERIES_BASEBOARD
config BOARD_FOO_CML_S bool "Foo Comet Lake S board" select BOARD_FOO_CFL_SERIES_BASEBOARD
config BOARD_FOO_CML_U bool "Foo Comet Lake U board" select BOARD_FOO_CFL_SERIES_BASEBOARD
Kconfig: config BOARD_FOO_CFL_SERIES_BASEBOARD def_bool n select HAVE_ACPI_RESUME ...
if BOARD_FOO_CFL_SERIES_BASEBOARD
config BOARD_FOO_CFL_H select BOARD_ROMSIZE_KB_16384 select SOC_INTEL_COFFEELAKE select SOC_INTEL_CANNONLAKE_PCH_H
config BOARD_FOO_CFL_S select BOARD_ROMSIZE_KB_32768 select SOC_INTEL_COFFEELAKE select SOC_INTEL_CANNONLAKE_PCH_H
config BOARD_FOO_CFL_U select BOARD_ROMSIZE_KB_32768 select SOC_INTEL_COFFEELAKE
config BOARD_FOO_WHL_U select BOARD_ROMSIZE_KB_16384 select SOC_INTEL_WHISKEYLAKE
config BOARD_FOO_CML_U select BOARD_ROMSIZE_KB_32768 select SOC_INTEL_COMETLAKE_1
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47051 )
Change subject: mb/purism/librem_cnl: Adjust in preparation for new variants ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... File src/mainboard/purism/librem_cnl/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... PS1, Line 4: select SOC_INTEL_WHISKEYLAKE
IMHO, this already looks quite messy already: https://github. […]
Looks good and is perfectly fine for me :) did you test it, though?
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47051 )
Change subject: mb/purism/librem_cnl: Adjust in preparation for new variants ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... File src/mainboard/purism/librem_cnl/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... PS1, Line 4: select SOC_INTEL_WHISKEYLAKE
But placing selects in `Kconfig.name` is already common practice for a year or so or maybe more.
I started doing it back in mid-2018 when I folded all the Skylake ChromeOS boards into variants of google/glados. I don't recall any objections at the time 😊
Are you 100% sure it will only be the SoC?
unless the ODM is lying, yes, as they claim the schematics are unchanged. I have both boards here, and in theory the only differences firmware-wise should be SoC (FSP), microcode, and VGA PCI ID. The devicetrees and GPIO defs should be the same, but aren't currently, and that's something I need to reconcile before it's ready to merge
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47051 )
Change subject: mb/purism/librem_cnl: Adjust in preparation for new variants ......................................................................
Patch Set 1:
I don't recall any objections at the time 😊
Well, not everything can be catched always ;) There's always room to improve, so why not start now? :-)
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47051 )
Change subject: mb/purism/librem_cnl: Adjust in preparation for new variants ......................................................................
Patch Set 1:
Patch Set 1:
I don't recall any objections at the time 😊
Well, not everything can be catched always ;) There's always room to improve, so why not start now? :-)
personally, I find it much more clear when board specific options are selected in `Kconfig.name`. If these were two variants derived from two different baseboards, then I'd agree it would make more sense to declare those in Kconfig and have the boards select those. But for a small number of board-specific selects? Not so much.
I'm aware I could just guard the SoC selection in Kconfig by board, but IMO that gets ugly when you have `select SOC_XXX if BOARD_X || BOARD_Y || BOARD_Z` and then the same for the other SoC
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47051 )
Change subject: mb/purism/librem_cnl: Adjust in preparation for new variants ......................................................................
Patch Set 1:
personally, I find it much more clear when board specific options are selected in `Kconfig.name`.
erm, you are bringing the argument from the very beginning up again... no, Kconfig.name is not the right place
I'm aware I could just guard the SoC selection in Kconfig by board, but IMO that gets ugly when you have `select SOC_XXX if BOARD_X || BOARD_Y || BOARD_Z` and then the same for the other SoC
huh? ofc you shouldn't do that. see Angel's example which boils down to a few lines Kconfig in your case
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47051 )
Change subject: mb/purism/librem_cnl: Adjust in preparation for new variants ......................................................................
Patch Set 1:
Patch Set 1:
huh? ofc you shouldn't do that. see Angel's example which boils down to a few lines Kconfig in your case
adding 2 new baseboards only to select the SoC, just to keep everything in Kconfig seems like a tortured exercise just to adhere to an arbitrary guideline, but I will relent and adjust
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47051 )
Change subject: mb/purism/librem_cnl: Adjust in preparation for new variants ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
huh? ofc you shouldn't do that. see Angel's example which boils down to a few lines Kconfig in your case
adding 2 new baseboards only to select the SoC, just to keep everything in Kconfig seems like a tortured exercise just to adhere to an arbitrary guideline, but I will relent and adjust
it's a ~10 LOC change; I don't see the "tortured exercise" ;-)
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47051 )
Change subject: mb/purism/librem_cnl: Adjust in preparation for new variants ......................................................................
Patch Set 1:
There you go, this is even easier and avoids selects in Kconfig.name at all:
Kconfig.name: config BOARD_PURISM_LIBREM_MINI bool "Librem Mini"
config BOARD_PURISM_LIBREM_XYZ bool "Librem XYZ"
Kconfig:
config BOARD_PURISM_BASEBOARD_LIBREM_WHL ...
config BOARD_PURISM_LIBREM_MINI select BOARD_PURISM_BASEBOARD_LIBREM_WHL select SOC_INTEL_WHISKEYLAKE
config BOARD_PURISM_LIBREM_XYZ select BOARD_PURISM_BASEBOARD_LIBREM_WHL select SOC_INTEL_COMETLAKE_1
if BOARD_PURISM_BASEBOARD_LIBREM_WHL ...
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47051 )
Change subject: mb/purism/librem_cnl: Adjust in preparation for new variants ......................................................................
Patch Set 1:
huh? ofc you shouldn't do that. see Angel's example which boils down to a few lines Kconfig in your case
adding 2 new baseboards only to select the SoC, just to keep everything in Kconfig seems like a tortured exercise just to adhere to an arbitrary guideline, but I will relent and adjust
it's a ~10 LOC change; I don't see the "tortured exercise" ;-)
Then look closer.
The original patch was correct by current coreboot standards. I'm not sure if a single of the proposed alternatives is. You seem to have the time to play around until Kconfig and your pickyness are happy. But that's not true for every author. I didn't read all comments but I doubt there's something like "if you have the time" along the lines.
So, the original patch was ok. What if you are happy with the next version but another reviewer isn't? With all the noise in the com- ments, there's no guarantee that a different version will get in easier.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47051 )
Change subject: mb/purism/librem_cnl: Adjust in preparation for new variants ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... File src/mainboard/purism/librem_cnl/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/47051/1/src/mainboard/purism/librem... PS1, Line 4: select SOC_INTEL_WHISKEYLAKE
But placing selects in `Kconfig.name` is already common practice for a year or so or maybe more. […]
Done
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47051 )
Change subject: mb/purism/librem_cnl: Adjust in preparation for new variants ......................................................................
Patch Set 1: Code-Review+2
Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47051 )
Change subject: mb/purism/librem_cnl: Adjust in preparation for new variants ......................................................................
mb/purism/librem_cnl: Adjust in preparation for new variants
- Move the SoC select to board config (vs baseboard config) - Qualify the VGA PCI ID and CBFS size values based on board selection - Move devicetree to variant dir and add Kconfig entry - Use a separate board_info.txt for the baseboard and each variant
Change-Id: I4764f2c1243ea49bd08e0735865cc3cb7a66441f Signed-off-by: Matt DeVillier matt.devillier@puri.sm Reviewed-on: https://review.coreboot.org/c/coreboot/+/47051 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Michael Niewöhner foss@mniewoehner.de --- M src/mainboard/purism/librem_cnl/Kconfig M src/mainboard/purism/librem_cnl/Kconfig.name M src/mainboard/purism/librem_cnl/board_info.txt A src/mainboard/purism/librem_cnl/variants/librem_mini/board_info.txt R src/mainboard/purism/librem_cnl/variants/librem_mini/devicetree.cb 5 files changed, 17 insertions(+), 5 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved Michael Niewöhner: Looks good to me, approved
diff --git a/src/mainboard/purism/librem_cnl/Kconfig b/src/mainboard/purism/librem_cnl/Kconfig index 38be380..464350c 100644 --- a/src/mainboard/purism/librem_cnl/Kconfig +++ b/src/mainboard/purism/librem_cnl/Kconfig @@ -9,7 +9,6 @@ select NO_UART_ON_SUPERIO select SOC_INTEL_COMMON_BLOCK_HDA select SOC_INTEL_COMMON_BLOCK_HDA_VERB - select SOC_INTEL_WHISKEYLAKE select SPD_READ_BY_WORD select USE_LEGACY_8254_TIMER
@@ -31,9 +30,13 @@ string default "librem_mini" if BOARD_PURISM_LIBREM_MINI
+config DEVICETREE + string + default "variants/$(CONFIG_VARIANT_DIR)/devicetree.cb" + config CBFS_SIZE hex - default 0x800000 + default 0x800000 if BOARD_PURISM_LIBREM_MINI
config MAX_CPUS int @@ -49,7 +52,7 @@
config VGA_BIOS_ID string - default "8086,3ea0" + default "8086,3ea0" if BOARD_PURISM_LIBREM_MINI
config PXE_ROM_ID string diff --git a/src/mainboard/purism/librem_cnl/Kconfig.name b/src/mainboard/purism/librem_cnl/Kconfig.name index 326165ba..83f1495 100644 --- a/src/mainboard/purism/librem_cnl/Kconfig.name +++ b/src/mainboard/purism/librem_cnl/Kconfig.name @@ -1,3 +1,4 @@ config BOARD_PURISM_LIBREM_MINI bool "Librem Mini" select BOARD_PURISM_BASEBOARD_LIBREM_CNL + select SOC_INTEL_WHISKEYLAKE diff --git a/src/mainboard/purism/librem_cnl/board_info.txt b/src/mainboard/purism/librem_cnl/board_info.txt index ca61edd..6c7620c 100644 --- a/src/mainboard/purism/librem_cnl/board_info.txt +++ b/src/mainboard/purism/librem_cnl/board_info.txt @@ -1,6 +1,6 @@ Vendor name: Purism -Board name: librem_cnl -Category: desktop +Board name: Librem Cannonlake baseboard +Category: misc Release year: 2020 ROM package: SOIC-8 ROM protocol: SPI diff --git a/src/mainboard/purism/librem_cnl/variants/librem_mini/board_info.txt b/src/mainboard/purism/librem_cnl/variants/librem_mini/board_info.txt new file mode 100644 index 0000000..843ff9f --- /dev/null +++ b/src/mainboard/purism/librem_cnl/variants/librem_mini/board_info.txt @@ -0,0 +1,8 @@ +Vendor name: Purism +Board name: Librem Mini +Category: desktop +Release year: 2020 +ROM package: SOIC-8 +ROM protocol: SPI +ROM socketed: n +Flashrom support: y diff --git a/src/mainboard/purism/librem_cnl/devicetree.cb b/src/mainboard/purism/librem_cnl/variants/librem_mini/devicetree.cb similarity index 100% rename from src/mainboard/purism/librem_cnl/devicetree.cb rename to src/mainboard/purism/librem_cnl/variants/librem_mini/devicetree.cb
-
Angel Pons (Code Review) -
Matt DeVillier (Code Review)
-
Michael Niewöhner (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)