Matt Papageorge 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 13:
(9 comments)
https://review.coreboot.org/c/coreboot/+/42567/7/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/42567/7/src/soc/amd/picasso/Kconfig... PS7, Line 373: default SPI_READ_MODE_NORMAL_66M if BOARD_AMD_MANDOLIN
Tried this, unfortunately I cannot 'select' an option from a 'choice' statement. […]
Done
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 391: "Normal Read (up to 66M)"
I need to get answers to this
Still trying to get answers for this in b/158755102 #14. Should we hold the change for this?
https://review.coreboot.org/c/coreboot/+/42567/11/src/soc/amd/picasso/Kconfi... File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/42567/11/src/soc/amd/picasso/Kconfi... PS11, Line 379: default SPI_READ_MODE_DUAL_IO_112 if DEFAULT_SPI_READ_MODE_DUAL_IO_112
If you want the default default to be 112, leave off the "if statement". […]
Ack
https://review.coreboot.org/c/coreboot/+/42567/11/src/soc/amd/picasso/Kconfi... PS11, Line 381: numbers numbers
X-Y-Z values?
Ack
https://review.coreboot.org/c/coreboot/+/42567/11/src/soc/amd/picasso/Kconfi... PS11, Line 456: SPI_MICRON_ALWAYS
So I'm still not certain of the actual impact of this. […]
Ack
https://review.coreboot.org/c/coreboot/+/42567/11/src/soc/amd/picasso/Kconfi... PS11, Line 471: default 1 if SPI_MICRON_ALWAYS
I agree, this should probably go in the src/soc/amd/common/block/spi/Kconfig file.
Done
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?
Done
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.
Done
https://review.coreboot.org/c/coreboot/+/42567/11/src/soc/amd/picasso/Makefi... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/42567/11/src/soc/amd/picasso/Makefi... PS11, Line 379: --soc-name "Picasso"
Does this apply for Pollock and Dali as well?
Yes, but only "Picasso" and "Raven" name strings are supported.
Originally I had parameters for passing in target family and model which would have been more explicit and better line up with what is used in the EFS spec. But feedback from the amdfw change indicated a strong preference for passing in the SOC name instead. For AMD kicker products like Pollock/Dali the main product name should be passed in.