Attention is currently required from: Martin Roth, Karthik Ramasubramanian, Felix Held. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56818 )
Change subject: soc/amd/common/block/spi: Don't update spi speed if EFS is changed ......................................................................
Patch Set 2:
(1 comment)
File src/soc/amd/common/block/spi/fch_spi.c:
https://review.coreboot.org/c/coreboot/+/56818/comment/709e3f9a_2d759672 PS2, Line 84: if (CONFIG(EM100)) : fch_spi_config_em100_modes(); : else : fch_spi_config_mb_modes();
Do you realize that EFS only configures some of the speeds, not all of them?
If EFS configures only some of the speeds, why is it okay to skip this when `read_efs_spi_speed() != CONFIG_EFS_SPI_SPEED`. Your question applies to the change you are making as well.
My point is that if you are skipping something based on the fact that EFS is configured differently, then it means that the EFS configuration must be honored. If there are more things that need to be configured than what is present in EFS, then this will have to be split into:
1. Read mode, Fast speed --> Part of EFS and so read EFS and configure from there. 2. TPM speed --> Not part of EFS and hence must be configured based on devicetree setting
For AltOpMode and normal speed --> I think this must be configured the same as Fast speed because those are applicable to the flash operations. So, it must be configured based on the speed setting in EFS. i.e. all non-TPM SPI operations must support the same speed. SO this code basically needs to be:
``` static void fch_spi_config_modes(void) { fch_spi_config_read_mode(); // Configure from EFS fch_spi_config_speed(); // Take fast speed from EFS --> use that for fast, altop and normal. Take TPM speed from device tree. } ```
Do you understand that this would allow us to use EM100 with our pre-built images instead of requiring a rebuild?
Yes, I understand that and hence I am saying that we can make that the default i.e. always rely on EFS and don't duplicate the configs in device tree. Because EFS configuration anyways overrules device tree config.
Do you realize that not all boards being used right now can run at the higher speeds that we're transitioning to?
Sure. My comment was just to ensure that we always honor EFS and drop the duplication in devicetree. So, whatever you have to do to support boards that cannot run at higher speeds would remain unchanged outside of coreboot.
With all of that in mind, and understanding that we can't JUST use EFS for configuration, do you still think that this should be dropped?
Yes, please see explanation above.