Martin Roth has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47719 )
Change subject: mb/google/zork: Remove 50ms WIFI delay ......................................................................
mb/google/zork: Remove 50ms WIFI delay
As a part of trying to get our boot time as low as possible, any delays in the code should try to be refactored out. This removes the 50ms delay in the WIFI sequence by enabling power and putting the wifi module into reset in bootblock, then bringing it out of reset in ramstage. This is significantly longer than the 50ms requirement. The reset GPIO was already being set high in ramstage, so that code didn't need to be added.
BUG=b:171513520 TEST=Boot on boards with different module types, WIFI works on both. BRANCH=Zork
Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: I211d3da338ad368d1f011f03cf7d05121c057075 --- M src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c 1 file changed, 2 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/47719/1
diff --git a/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c b/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c index 14b85b0..73f6314 100644 --- a/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c +++ b/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c @@ -199,8 +199,8 @@ * Configure WiFi GPIOs such that: * - WIFI_AUX_RESET_L is configured first to assert PERST# to WiFi device. * - Enable power to WiFi using EN_PWR_WIFI_L. - * - Wait for 50ms after power to WiFi is enabled. - * - Deassert WIFI_AUX_RESET_L. + * - Wait for >50ms after power to WiFi is enabled. (Time between bootblock & ramstage) + * - Deassert WIFI_AUX_RESET_L in mainboard_configure_gpios */ static const struct soc_amd_gpio v3_wifi_table[] = { /* WIFI_AUX_RESET_L */ @@ -210,8 +210,6 @@ }; program_gpios(v3_wifi_table, ARRAY_SIZE(v3_wifi_table));
- mdelay(50); - gpio_set(GPIO_86, 1); }
static void wifi_power_reset_configure_active_high_power(void)
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47719 )
Change subject: mb/google/zork: Remove 50ms WIFI delay ......................................................................
Patch Set 1:
(2 comments)
the code looks good to me
https://review.coreboot.org/c/coreboot/+/47719/1/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c:
https://review.coreboot.org/c/coreboot/+/47719/1/src/mainboard/google/zork/v... PS1, Line 203: * - Deassert WIFI_AUX_RESET_L in mainboard_configure_gpios "WIFI_AUX_RESET_L gets deasserted later in mainboard_configure_gpios in ramstage" maybe?
https://review.coreboot.org/c/coreboot/+/47719/1/src/mainboard/google/zork/v... PS1, Line 276: __weak void variant_pcie_gpio_configure(void) out of scope for this patch, but i'd like this function to be renamed to variant_pcie_early_gpio_configure or similar so that it's clearer from the function name that this is the part of the gpio setup that's done in bootblock
Eric Peers has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47719 )
Change subject: mb/google/zork: Remove 50ms WIFI delay ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47719/1/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c:
https://review.coreboot.org/c/coreboot/+/47719/1/src/mainboard/google/zork/v... PS1, Line 213: mdelay(50); in order to guarantee the mdelay, do we need to set a timer and inject a delay later? I would guess it would never trigger, but there's nothing in the ramstage that's explicitly guaranteeing this contract right?
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47719 )
Change subject: mb/google/zork: Remove 50ms WIFI delay ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47719/1/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c:
https://review.coreboot.org/c/coreboot/+/47719/1/src/mainboard/google/zork/v... PS1, Line 213: mdelay(50);
in order to guarantee the mdelay, do we need to set a timer and inject a delay later? I would guess […]
Not a bad idea, however I don't think we're anywhere near the edge now. In an actual implementation, it would need some care for doing it properly. romstage and ramstage don't share data, i.e. you can't just add a static variable in this file to store a TSC value to be checked later.
Hello build bot (Jenkins), Furquan Shaikh, Marshall Dawson, Eric Peers, Rob Barnes, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47719
to look at the new patch set (#2).
Change subject: mb/google/zork: Remove 50ms WIFI delay ......................................................................
mb/google/zork: Remove 50ms WIFI delay
As a part of trying to get our boot time as low as possible, any delays in the code should try to be refactored out. This removes the 50ms delay in the WIFI sequence by enabling power and putting the wifi module into reset in bootblock, then bringing it out of reset in ramstage. This is significantly longer than the 50ms requirement. The reset GPIO was already being set high in ramstage, so that code didn't need to be added.
BUG=b:171513520 TEST=Boot on boards with different module types, WIFI works on both. BRANCH=Zork
Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: I211d3da338ad368d1f011f03cf7d05121c057075 --- M src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c 1 file changed, 2 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/47719/2
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47719 )
Change subject: mb/google/zork: Remove 50ms WIFI delay ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/47719/1/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c:
https://review.coreboot.org/c/coreboot/+/47719/1/src/mainboard/google/zork/v... PS1, Line 213: mdelay(50);
Not a bad idea, however I don't think we're anywhere near the edge now. […]
The time between the end of bootblock to the start of ramstage is 224ms. Since we only need 50ms, I think we're good with no timer.
https://review.coreboot.org/c/coreboot/+/47719/1/src/mainboard/google/zork/v... PS1, Line 203: * - Deassert WIFI_AUX_RESET_L in mainboard_configure_gpios
"WIFI_AUX_RESET_L gets deasserted later in mainboard_configure_gpios in ramstage" maybe?
Done
https://review.coreboot.org/c/coreboot/+/47719/1/src/mainboard/google/zork/v... PS1, Line 276: __weak void variant_pcie_gpio_configure(void)
out of scope for this patch, but i'd like this function to be renamed to variant_pcie_early_gpio_con […]
Tracked in b/173626527
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47719 )
Change subject: mb/google/zork: Remove 50ms WIFI delay ......................................................................
Patch Set 2: Code-Review+2
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47719 )
Change subject: mb/google/zork: Remove 50ms WIFI delay ......................................................................
mb/google/zork: Remove 50ms WIFI delay
As a part of trying to get our boot time as low as possible, any delays in the code should try to be refactored out. This removes the 50ms delay in the WIFI sequence by enabling power and putting the wifi module into reset in bootblock, then bringing it out of reset in ramstage. This is significantly longer than the 50ms requirement. The reset GPIO was already being set high in ramstage, so that code didn't need to be added.
BUG=b:171513520 TEST=Boot on boards with different module types, WIFI works on both. BRANCH=Zork
Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: I211d3da338ad368d1f011f03cf7d05121c057075 Reviewed-on: https://review.coreboot.org/c/coreboot/+/47719 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c 1 file changed, 2 insertions(+), 4 deletions(-)
Approvals: build bot (Jenkins): Verified Marshall Dawson: Looks good to me, approved
diff --git a/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c b/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c index 14b85b0..818c39b 100644 --- a/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c +++ b/src/mainboard/google/zork/variants/baseboard/gpio_baseboard_trembyle.c @@ -199,8 +199,8 @@ * Configure WiFi GPIOs such that: * - WIFI_AUX_RESET_L is configured first to assert PERST# to WiFi device. * - Enable power to WiFi using EN_PWR_WIFI_L. - * - Wait for 50ms after power to WiFi is enabled. - * - Deassert WIFI_AUX_RESET_L. + * - Wait for >50ms after power to WiFi is enabled. (Time between bootblock & ramstage) + * - WIFI_AUX_RESET_L gets deasserted later in mainboard_configure_gpios in ramstage */ static const struct soc_amd_gpio v3_wifi_table[] = { /* WIFI_AUX_RESET_L */ @@ -210,8 +210,6 @@ }; program_gpios(v3_wifi_table, ARRAY_SIZE(v3_wifi_table));
- mdelay(50); - gpio_set(GPIO_86, 1); }
static void wifi_power_reset_configure_active_high_power(void)
-
Eric Peers (Code Review) -
Felix Held (Code Review)
-
Marshall Dawson (Code Review)
-
Martin Roth (Code Review)