Attention is currently required from: Furquan Shaikh, Karthik Ramasubramanian, Felix Held.
Martin Roth 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/62c50dc8_505ece06
PS2, Line 84: if (CONFIG(EM100))
: fch_spi_config_em100_modes();
: else
: fch_spi_config_mb_modes();
> Felix commented on https://review.coreboot. […]
Ok, I think we're all in agreement.
--
To view, visit https://review.coreboot.org/c/coreboot/+/56818
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I278768e361499919666d07e1dd92d83c2390035e
Gerrit-Change-Number: 56818
Gerrit-PatchSet: 2
Gerrit-Owner: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Furquan Shaikh <furquan(a)google.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Furquan Shaikh <furquan(a)google.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Fri, 06 Aug 2021 21:22:25 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com>
Comment-In-Reply-To: Martin Roth <martinroth(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Furquan Shaikh, Martin Roth, Karthik Ramasubramanian.
Felix Held 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/fab5baf6_e6d8f0da
PS2, Line 84: if (CONFIG(EM100))
: fch_spi_config_em100_modes();
: else
: fch_spi_config_mb_modes();
> Felix commented on https://review.coreboot. […]
normal speed needs to be different from fast speed. i'm not sure if altio needs to have the same speed as fast speed, but apart from that thing i'm not sure about, the proposal sounds like the way to go to me
--
To view, visit https://review.coreboot.org/c/coreboot/+/56818
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I278768e361499919666d07e1dd92d83c2390035e
Gerrit-Change-Number: 56818
Gerrit-PatchSet: 2
Gerrit-Owner: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Furquan Shaikh <furquan(a)google.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Furquan Shaikh <furquan(a)google.com>
Gerrit-Attention: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Fri, 06 Aug 2021 21:10:02 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com>
Comment-In-Reply-To: Martin Roth <martinroth(a)google.com>
Gerrit-MessageType: comment
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/a5c56781_b6cd0d1b
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? […]
Felix commented on https://review.coreboot.org/c/coreboot/+/56645/comment/95dbc5d7_51464870/ that normal speed might have to be different than fast speed. If that is the case, I think it is better to have all of this in Kconfig and use required configs from the respective places:
1. Read mode, Fast speed --> Define in Kconfig, part of EFS, coreboot configures using EFS.
2. TPM speed, Normal speed --> Define in Kconfig, coreboot configures using Kconfig.
3. AltIO speed --> If this has to be the same as fast speed, then it can follow #1.
Basically, all configs can be defined in Kconfig by mainboard user. Common code can take care of managing that and using it appropriately.
--
To view, visit https://review.coreboot.org/c/coreboot/+/56818
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I278768e361499919666d07e1dd92d83c2390035e
Gerrit-Change-Number: 56818
Gerrit-PatchSet: 2
Gerrit-Owner: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Furquan Shaikh <furquan(a)google.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Fri, 06 Aug 2021 21:01:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com>
Comment-In-Reply-To: Martin Roth <martinroth(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Raul Rangel, Karthik Ramasubramanian, Felix Held.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56645 )
Change subject: mb/google/guybrush: Switch from 33MHz to 66MHz SPI Speed
......................................................................
Patch Set 3:
(1 comment)
File src/mainboard/google/guybrush/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/56645/comment/876fc694_255aceeb
PS3, Line 13: 33M
> i strongly doubt that 66mhz for the normal read mode will work. […]
Are you referring to the 8 dummy cycles that are added for fast read command? What is the maximum supported value for normal_speed? What about altio? What operations are performed using altio speed? Have we confirmed that 66M is fine for that?
--
To view, visit https://review.coreboot.org/c/coreboot/+/56645
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Icc5c0580ed0d19f1fffce59df3888dd7963255a1
Gerrit-Change-Number: 56645
Gerrit-PatchSet: 3
Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Fri, 06 Aug 2021 20:58:00 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com>
Comment-In-Reply-To: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-MessageType: comment
Attention is currently required from: Raul Rangel, Furquan Shaikh, Karthik Ramasubramanian.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56645 )
Change subject: mb/google/guybrush: Switch from 33MHz to 66MHz SPI Speed
......................................................................
Patch Set 3:
(1 comment)
File src/mainboard/google/guybrush/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/56645/comment/56475fc6_3ad87528
PS3, Line 13: 33M
> Why is this not changed?
i strongly doubt that 66mhz for the normal read mode will work. fast mode gives the flash chip 8 more bit times to have the data ready which allows faster spi speeds
--
To view, visit https://review.coreboot.org/c/coreboot/+/56645
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Icc5c0580ed0d19f1fffce59df3888dd7963255a1
Gerrit-Change-Number: 56645
Gerrit-PatchSet: 3
Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Furquan Shaikh <furquan(a)google.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Fri, 06 Aug 2021 20:46:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Raul Rangel, Karthik Ramasubramanian, Felix Held.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56645 )
Change subject: mb/google/guybrush: Switch from 33MHz to 66MHz SPI Speed
......................................................................
Patch Set 3:
(1 comment)
File src/mainboard/google/guybrush/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/56645/comment/95dbc5d7_51464870
PS3, Line 13: 33M
Why is this not changed?
--
To view, visit https://review.coreboot.org/c/coreboot/+/56645
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Icc5c0580ed0d19f1fffce59df3888dd7963255a1
Gerrit-Change-Number: 56645
Gerrit-PatchSet: 3
Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Fri, 06 Aug 2021 20:40:02 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
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.
--
To view, visit https://review.coreboot.org/c/coreboot/+/56818
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I278768e361499919666d07e1dd92d83c2390035e
Gerrit-Change-Number: 56818
Gerrit-PatchSet: 2
Gerrit-Owner: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Furquan Shaikh <furquan(a)google.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Fri, 06 Aug 2021 20:36:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Martin Roth <martinroth(a)google.com>
Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Martin Roth, Furquan Shaikh, Karthik Ramasubramanian.
Felix Held 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/be38e840_36e8d1f7
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? […]
yep, the EFS doesn't have all info we need here. having half of the settings in Kconfig that will be put into EFS and the other half in the devicetree doesn't sound like a good and easy to understand solution either
--
To view, visit https://review.coreboot.org/c/coreboot/+/56818
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I278768e361499919666d07e1dd92d83c2390035e
Gerrit-Change-Number: 56818
Gerrit-PatchSet: 2
Gerrit-Owner: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Furquan Shaikh <furquan(a)google.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Furquan Shaikh <furquan(a)google.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Fri, 06 Aug 2021 20:34:11 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Martin Roth <martinroth(a)google.com>
Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com>
Gerrit-MessageType: comment