Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42567 )
Change subject: soc/amd/picasso: Populate EFS SPI values from Kconfig options ......................................................................
Patch Set 8:
(9 comments)
https://review.coreboot.org/c/coreboot/+/42567/8/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/42567/8/src/soc/amd/picasso/Kconfig... PS8, Line 370: choice SPI_READ_MODE As I mention below, you could just get rid of the choice menu and have one option that lets you directly select the value.
https://review.coreboot.org/c/coreboot/+/42567/8/src/soc/amd/picasso/Kconfig... PS8, Line 373: default SPI_READ_MODE_NORMAL_66M if BOARD_AMD_MANDOLIN 1) this will never have an effect since the first default is the one that gets used. 2) Can you override this in the mandoin board instead of here?
https://doc.coreboot.org/getting_started/kconfig.html?highlight=kconfig#defa...
https://review.coreboot.org/c/coreboot/+/42567/8/src/soc/amd/picasso/Kconfig... PS8, Line 379: (1-1-2) Maybe address what each of these numbers mean in the help somewhere?
“1-1-2" means command, address and data are transmitted through 1 wire, 1 wire and 2 wire, respectively.
https://review.coreboot.org/c/coreboot/+/42567/8/src/soc/amd/picasso/Kconfig... PS8, Line 391: "Normal Read (up to 66M)" Does this mean 1-1-1?
https://review.coreboot.org/c/coreboot/+/42567/8/src/soc/amd/picasso/Kconfig... PS8, Line 397: Add EFS_SPI_READ_MODE here so it's not done in the makefile?
# Do not override this value in the mainboard - override the SPI_READ_MODE value instead. config EFS_SPI_READ_MODE int default 0 if SPI_READ_MODE_NORMAL_33M default 2 if SPI_READ_MODE_DUAL_IO_112 default 3 if SPI_READ_MODE_QUAD_IO_114 ...
Optionally, you can get rid of the choice and just use this value directly, with all of the values in the help text.
https://review.coreboot.org/c/coreboot/+/42567/8/src/soc/amd/picasso/Kconfig... PS8, Line 429: Should be set if Micron parts are used for the BIOS SPI chip I'd like to understand more about this option. What are the downsides to selecting always? If there are no downsides, why have the option?
https://review.coreboot.org/c/coreboot/+/42567/8/src/soc/amd/picasso/Kconfig... PS8, Line 435: are always be used "may always be used?"
https://review.coreboot.org/c/coreboot/+/42567/8/src/soc/amd/picasso/Makefil... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/42567/8/src/soc/amd/picasso/Makefil... PS8, Line 326: EFS_SPI_READ_MODE=0 Why not do all of these in the Kconfig instead of in the makefile?
https://review.coreboot.org/c/coreboot/+/42567/8/src/soc/amd/picasso/Makefil... PS8, Line 340: EFS_SPI_READ_MODE If you put all of that in the Kconfig file, you can just use CONFIG_EFS_SPI_READ_MODE here.