Change in coreboot[master]: soc/amd/picasso: don't apply unconfigured USB2 PHY tune parameters
Felix Held has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43781 )
Change subject: soc/amd/picasso: don't apply unconfigured USB2 PHY tune parameters ......................................................................
soc/amd/picasso: don't apply unconfigured USB2 PHY tune parameters
Since FSP pre-populates the UPD struct with the non-zero default values, coreboot shouldn't set them to zero in the case that they aren't configured in the board's devicetree. Since all parameters being zero is a valid case, this patch adds another devicetree option that applying the devicetree settings for the USB2 PHY tuning depends on being set.
Change-Id: I66e5811ce64298b0644d2881420634a8ce1379d7 Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb M src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb M src/soc/amd/picasso/chip.h M src/soc/amd/picasso/fsp_params.c 4 files changed, 13 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/43781/1
diff --git a/src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb b/src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb index e355b5b..4a01a12 100644 --- a/src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb +++ b/src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb @@ -51,6 +51,8 @@
register "xhci0_force_gen1" = "0"
+ register "has_usb2_phy_tune_params" = "1" + # Controller0 Port0 Default register "usb_2_port_0_tune_params" = "{ .com_pds_tune = 0x03, diff --git a/src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb b/src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb index 283e7a0..986c444 100644 --- a/src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb +++ b/src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb @@ -51,6 +51,8 @@
register "xhci0_force_gen1" = "0"
+ register "has_usb2_phy_tune_params" = "1" + # Controller0 Port0 Default register "usb_2_port_0_tune_params" = "{ .com_pds_tune = 0x03, diff --git a/src/soc/amd/picasso/chip.h b/src/soc/amd/picasso/chip.h index 3bcd3cc..d3c9a07 100644 --- a/src/soc/amd/picasso/chip.h +++ b/src/soc/amd/picasso/chip.h @@ -133,13 +133,13 @@
uint8_t xhci0_force_gen1;
+ uint8_t has_usb2_phy_tune_params; struct usb2_phy_tune usb_2_port_0_tune_params; struct usb2_phy_tune usb_2_port_1_tune_params; struct usb2_phy_tune usb_2_port_2_tune_params; struct usb2_phy_tune usb_2_port_3_tune_params; struct usb2_phy_tune usb_2_port_4_tune_params; struct usb2_phy_tune usb_2_port_5_tune_params; - };
typedef struct soc_amd_picasso_config config_t; diff --git a/src/soc/amd/picasso/fsp_params.c b/src/soc/amd/picasso/fsp_params.c index d07c384..d280bff 100644 --- a/src/soc/amd/picasso/fsp_params.c +++ b/src/soc/amd/picasso/fsp_params.c @@ -102,12 +102,14 @@
scfg->xhci0_force_gen1 = cfg->xhci0_force_gen1;
- memcpy(scfg->fch_usb_2_port0_phy_tune, &cfg->usb_2_port_0_tune_params, num); - memcpy(scfg->fch_usb_2_port1_phy_tune, &cfg->usb_2_port_1_tune_params, num); - memcpy(scfg->fch_usb_2_port2_phy_tune, &cfg->usb_2_port_2_tune_params, num); - memcpy(scfg->fch_usb_2_port3_phy_tune, &cfg->usb_2_port_3_tune_params, num); - memcpy(scfg->fch_usb_2_port4_phy_tune, &cfg->usb_2_port_4_tune_params, num); - memcpy(scfg->fch_usb_2_port5_phy_tune, &cfg->usb_2_port_5_tune_params, num); + if (cfg->has_usb2_phy_tune_params) { + memcpy(scfg->fch_usb_2_port0_phy_tune, &cfg->usb_2_port_0_tune_params, num); + memcpy(scfg->fch_usb_2_port1_phy_tune, &cfg->usb_2_port_1_tune_params, num); + memcpy(scfg->fch_usb_2_port2_phy_tune, &cfg->usb_2_port_2_tune_params, num); + memcpy(scfg->fch_usb_2_port3_phy_tune, &cfg->usb_2_port_3_tune_params, num); + memcpy(scfg->fch_usb_2_port4_phy_tune, &cfg->usb_2_port_4_tune_params, num); + memcpy(scfg->fch_usb_2_port5_phy_tune, &cfg->usb_2_port_5_tune_params, num); + } }
void platform_fsp_silicon_init_params_cb(FSPS_UPD *supd)
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43781 )
Change subject: soc/amd/picasso: don't apply unconfigured USB2 PHY tune parameters ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/43781/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43781/1//COMMIT_MSG@11 PS1, Line 11: configured in the board's devicetree. Since all parameters being zero is That's assuming default values are valid. They never are...
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43781 )
Change subject: soc/amd/picasso: don't apply unconfigured USB2 PHY tune parameters ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43781/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43781/1//COMMIT_MSG@11 PS1, Line 11: configured in the board's devicetree. Since all parameters being zero is
That's assuming default values are valid. They never are...
The default UPD values added in the FSP build process are certainly less wrong than setting all fields to 0. I don't expect them to be right for every possible mainboard which is probably also why the devicetree settings exist. When the settings are missing from the devicetree like currently for Mandolin, I strongly prefer having some defaults instead of writing 0 to every field. Would you prefer if the coreboot code explicitly sets the fields to the defaults from FSP when the board doesn't have the corresponding devicetree entries?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43781 )
Change subject: soc/amd/picasso: don't apply unconfigured USB2 PHY tune parameters ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43781/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43781/1//COMMIT_MSG@11 PS1, Line 11: configured in the board's devicetree. Since all parameters being zero is
The default UPD values added in the FSP build process are certainly less wrong than setting all fiel […]
Defaults can change. I was just noting in my experience that the defaults are normally wrong because one doesn't know the right values for the implementation (board design). "less wrong" -> sometimes working -> flaky and hard to understand failures seems better than "nothing works" which leads to people performing the proper design tuning. That's my opinion, but we can go this route. It's fine by me.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43781 )
Change subject: soc/amd/picasso: don't apply unconfigured USB2 PHY tune parameters ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43781/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43781/1//COMMIT_MSG@11 PS1, Line 11: configured in the board's devicetree. Since all parameters being zero is
Defaults can change. […]
I do agree that just using defaults and hoping for the best isn't good practice and that boards should use optimized values. Found a problem with the next patch in the patch train anyway that prevents us from using the defaults, so I'll just convert the devicetree settings to the changed format in the patch that reworks the data structures and handling.
Surprisingly having everything set to 0 didn't break USB2 on mandolin.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43781 )
Change subject: soc/amd/picasso: don't apply unconfigured USB2 PHY tune parameters ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/43781/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43781/1//COMMIT_MSG@11 PS1, Line 11: configured in the board's devicetree. Since all parameters being zero is
I do agree that just using defaults and hoping for the best isn't good practice and that boards shou […]
For retrofitted coreboot ports (if that would be doable with picasso), determining the correct tuning settings is a bit hard. While defaults may not be the best, they're less likely to break something than all zeroes.
Hello build bot (Jenkins), Angel Pons, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43781
to look at the new patch set (#2).
Change subject: soc/amd/picasso: don't apply unconfigured USB2 PHY tune parameters ......................................................................
soc/amd/picasso: don't apply unconfigured USB2 PHY tune parameters
Since FSP pre-populates the UPD struct with the non-zero default values, coreboot shouldn't set them to zero in the case that they aren't configured in the board's devicetree. Since all parameters being zero is a valid case, this patch adds another devicetree option that applying the devicetree settings for the USB2 PHY tuning depends on being set.
BUG=b:161923068
Change-Id: I66e5811ce64298b0644d2881420634a8ce1379d7 Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb M src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb M src/soc/amd/picasso/chip.h M src/soc/amd/picasso/fsp_params.c 4 files changed, 13 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/43781/2
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43781 )
Change subject: soc/amd/picasso: don't apply unconfigured USB2 PHY tune parameters ......................................................................
soc/amd/picasso: don't apply unconfigured USB2 PHY tune parameters
Since FSP pre-populates the UPD struct with the non-zero default values, coreboot shouldn't set them to zero in the case that they aren't configured in the board's devicetree. Since all parameters being zero is a valid case, this patch adds another devicetree option that applying the devicetree settings for the USB2 PHY tuning depends on being set.
BUG=b:161923068
Change-Id: I66e5811ce64298b0644d2881420634a8ce1379d7 Signed-off-by: Felix Held felix-coreboot@felixheld.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/43781 Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Angel Pons th3fanbus@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb M src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb M src/soc/amd/picasso/chip.h M src/soc/amd/picasso/fsp_params.c 4 files changed, 13 insertions(+), 7 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Angel Pons: Looks good to me, approved
diff --git a/src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb b/src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb index e355b5b..4a01a12 100644 --- a/src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb +++ b/src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb @@ -51,6 +51,8 @@
register "xhci0_force_gen1" = "0"
+ register "has_usb2_phy_tune_params" = "1" + # Controller0 Port0 Default register "usb_2_port_0_tune_params" = "{ .com_pds_tune = 0x03, diff --git a/src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb b/src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb index 283e7a0..986c444 100644 --- a/src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb +++ b/src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb @@ -51,6 +51,8 @@
register "xhci0_force_gen1" = "0"
+ register "has_usb2_phy_tune_params" = "1" + # Controller0 Port0 Default register "usb_2_port_0_tune_params" = "{ .com_pds_tune = 0x03, diff --git a/src/soc/amd/picasso/chip.h b/src/soc/amd/picasso/chip.h index 3bcd3cc..d3c9a07 100644 --- a/src/soc/amd/picasso/chip.h +++ b/src/soc/amd/picasso/chip.h @@ -133,13 +133,13 @@
uint8_t xhci0_force_gen1;
+ uint8_t has_usb2_phy_tune_params; struct usb2_phy_tune usb_2_port_0_tune_params; struct usb2_phy_tune usb_2_port_1_tune_params; struct usb2_phy_tune usb_2_port_2_tune_params; struct usb2_phy_tune usb_2_port_3_tune_params; struct usb2_phy_tune usb_2_port_4_tune_params; struct usb2_phy_tune usb_2_port_5_tune_params; - };
typedef struct soc_amd_picasso_config config_t; diff --git a/src/soc/amd/picasso/fsp_params.c b/src/soc/amd/picasso/fsp_params.c index d07c384..d280bff 100644 --- a/src/soc/amd/picasso/fsp_params.c +++ b/src/soc/amd/picasso/fsp_params.c @@ -102,12 +102,14 @@
scfg->xhci0_force_gen1 = cfg->xhci0_force_gen1;
- memcpy(scfg->fch_usb_2_port0_phy_tune, &cfg->usb_2_port_0_tune_params, num); - memcpy(scfg->fch_usb_2_port1_phy_tune, &cfg->usb_2_port_1_tune_params, num); - memcpy(scfg->fch_usb_2_port2_phy_tune, &cfg->usb_2_port_2_tune_params, num); - memcpy(scfg->fch_usb_2_port3_phy_tune, &cfg->usb_2_port_3_tune_params, num); - memcpy(scfg->fch_usb_2_port4_phy_tune, &cfg->usb_2_port_4_tune_params, num); - memcpy(scfg->fch_usb_2_port5_phy_tune, &cfg->usb_2_port_5_tune_params, num); + if (cfg->has_usb2_phy_tune_params) { + memcpy(scfg->fch_usb_2_port0_phy_tune, &cfg->usb_2_port_0_tune_params, num); + memcpy(scfg->fch_usb_2_port1_phy_tune, &cfg->usb_2_port_1_tune_params, num); + memcpy(scfg->fch_usb_2_port2_phy_tune, &cfg->usb_2_port_2_tune_params, num); + memcpy(scfg->fch_usb_2_port3_phy_tune, &cfg->usb_2_port_3_tune_params, num); + memcpy(scfg->fch_usb_2_port4_phy_tune, &cfg->usb_2_port_4_tune_params, num); + memcpy(scfg->fch_usb_2_port5_phy_tune, &cfg->usb_2_port_5_tune_params, num); + } }
void platform_fsp_silicon_init_params_cb(FSPS_UPD *supd)
-
Aaron Durbin (Code Review) -
Angel Pons (Code Review)
-
Felix Held (Code Review)