Change in coreboot[master]: soc/amd/picasso/soc_util: add function to detect defeatured SKU
Felix Held has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42833 )
Change subject: soc/amd/picasso/soc_util: add function to detect defeatured SKU ......................................................................
soc/amd/picasso/soc_util: add function to detect defeatured SKU
Change-Id: I9eb57595da6f806305552128b0c077ceeb7c4661 Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/soc/amd/picasso/include/soc/soc_util.h M src/soc/amd/picasso/soc_util.c 2 files changed, 8 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/42833/1
diff --git a/src/soc/amd/picasso/include/soc/soc_util.h b/src/soc/amd/picasso/include/soc/soc_util.h index 2459207..e7f13d4 100644 --- a/src/soc/amd/picasso/include/soc/soc_util.h +++ b/src/soc/amd/picasso/include/soc/soc_util.h @@ -39,4 +39,7 @@ /* function to determine the iGPU type */ bool soc_is_raven2(void);
+/* function to determine if SoC has less USB and PCIe lanes */ +bool soc_is_defeatured_sku(void); + #endif /* __PICASSO_SOC_UTIL_H__ */ diff --git a/src/soc/amd/picasso/soc_util.c b/src/soc/amd/picasso/soc_util.c index 2aa9daa..8a1f8ef 100644 --- a/src/soc/amd/picasso/soc_util.c +++ b/src/soc/amd/picasso/soc_util.c @@ -208,3 +208,8 @@ { return get_silicon_type() == SILICON_RV2; } + +bool soc_is_defeatured_sku(void) +{ + return get_silicon_type() == SILICON_RV2 || get_soc_type() == SOC_DALI; +}
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42833 )
Change subject: soc/amd/picasso/soc_util: add function to detect defeatured SKU ......................................................................
Patch Set 1:
this might be helpful for the XHCI patch
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42833 )
Change subject: soc/amd/picasso/soc_util: add function to detect defeatured SKU ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42833/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42833/1//COMMIT_MSG@8 PS1, Line 8: nit: maybe mention what makes a SKU defeatured?
https://review.coreboot.org/c/coreboot/+/42833/1/src/soc/amd/picasso/soc_uti... File src/soc/amd/picasso/soc_util.c:
https://review.coreboot.org/c/coreboot/+/42833/1/src/soc/amd/picasso/soc_uti... PS1, Line 214: || Sanity check: Is this correct? Shouldn't this be an AND instead?
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42833 )
Change subject: soc/amd/picasso/soc_util: add function to detect defeatured SKU ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42833/1/src/soc/amd/picasso/soc_uti... File src/soc/amd/picasso/soc_util.c:
https://review.coreboot.org/c/coreboot/+/42833/1/src/soc/amd/picasso/soc_uti... PS1, Line 214: ||
Sanity check: Is this correct? Shouldn't this be an AND instead?
nope, this needs to be an or; this is to catch the defeatured-PCO Dali SKUs
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42833 )
Change subject: soc/amd/picasso/soc_util: add function to detect defeatured SKU ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42833/1/src/soc/amd/picasso/soc_uti... File src/soc/amd/picasso/soc_util.c:
https://review.coreboot.org/c/coreboot/+/42833/1/src/soc/amd/picasso/soc_uti... PS1, Line 212: bool soc_is_defeatured_sku(void) At least I do not understand, what *defeatured* means. Even after reading https://en.wiktionary.org/wiki/defeatured. Is that a common term?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42833 )
Change subject: soc/amd/picasso/soc_util: add function to detect defeatured SKU ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/42833/1/src/soc/amd/picasso/soc_uti... File src/soc/amd/picasso/soc_util.c:
https://review.coreboot.org/c/coreboot/+/42833/1/src/soc/amd/picasso/soc_uti... PS1, Line 214: ||
nope, this needs to be an or; this is to catch the defeatured-PCO Dali SKUs
Ack, looks like either condition results in less lanes
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42833 )
Change subject: soc/amd/picasso/soc_util: add function to detect defeatured SKU ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42833/1/src/soc/amd/picasso/soc_uti... File src/soc/amd/picasso/soc_util.c:
https://review.coreboot.org/c/coreboot/+/42833/1/src/soc/amd/picasso/soc_uti... PS1, Line 212: bool soc_is_defeatured_sku(void)
At least I do not understand, what *defeatured* means. Even after reading https://en.wiktionary. […]
It means that it has less features. In this particular case, refer to the comment in the header:
/* function to determine if SoC has less USB and PCIe lanes */
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42833 )
Change subject: soc/amd/picasso/soc_util: add function to detect defeatured SKU ......................................................................
Patch Set 1: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/42833/1/src/soc/amd/picasso/soc_uti... File src/soc/amd/picasso/soc_util.c:
https://review.coreboot.org/c/coreboot/+/42833/1/src/soc/amd/picasso/soc_uti... PS1, Line 212: bool soc_is_defeatured_sku(void)
At least I do not understand, what *defeatured* means. Even after reading https://en.wiktionary. […]
Which features are missing? If it's just xhci1? We could change to something like soc_has_xhci1 and invert the logic?
https://review.coreboot.org/c/coreboot/+/42833/1/src/soc/amd/picasso/soc_uti... PS1, Line 214: SILICON_RV2 Could be reduced to 'soc_is_raven2() || soc_is_dali()'
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42833 )
Change subject: soc/amd/picasso/soc_util: add function to detect defeatured SKU ......................................................................
Patch Set 1:
(2 comments)
if you have some better name than defeatured, i'm all ears
https://review.coreboot.org/c/coreboot/+/42833/1/src/soc/amd/picasso/soc_uti... File src/soc/amd/picasso/soc_util.c:
https://review.coreboot.org/c/coreboot/+/42833/1/src/soc/amd/picasso/soc_uti... PS1, Line 212: bool soc_is_defeatured_sku(void)
Which features are missing? If it's just xhci1? We could change to something like soc_has_xhci1 and […]
less PCIe lanes, no second XHCI controller, less USB3 ports, no 4th display port, only 2C/4T and maybe something more
https://review.coreboot.org/c/coreboot/+/42833/1/src/soc/amd/picasso/soc_uti... PS1, Line 214: SILICON_RV2
Could be reduced to 'soc_is_raven2() || soc_is_dali()'
it would be equivalent, but i'm not sure if i should use the functions here that are mostly there for convenience when used in the soc/mainboard code. If we ignore the whole CPUID business that i only kept to still guess the types correctly in maybe 90% of the cases when the HOB is missing, this would be is_rv2_silicon() || is_mystery_silicon(). not sure if I should just rip the whole CPUID stuff out of the detection logic
Hello build bot (Jenkins), Angel Pons, Rob Barnes,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42833
to look at the new patch set (#2).
Change subject: soc/amd/picasso/soc_util: add function to detect defeatured SKU ......................................................................
soc/amd/picasso/soc_util: add function to detect defeatured SKU
A defeatured SKU has less PCIe, USB3 and Displayport connectivity.
Change-Id: I9eb57595da6f806305552128b0c077ceeb7c4661 Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/soc/amd/picasso/include/soc/soc_util.h M src/soc/amd/picasso/soc_util.c 2 files changed, 8 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/42833/2
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42833 )
Change subject: soc/amd/picasso/soc_util: add function to detect defeatured SKU ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42833/1/src/soc/amd/picasso/soc_uti... File src/soc/amd/picasso/soc_util.c:
https://review.coreboot.org/c/coreboot/+/42833/1/src/soc/amd/picasso/soc_uti... PS1, Line 212: bool soc_is_defeatured_sku(void)
less PCIe lanes, no second XHCI controller, less USB3 ports, no 4th display port, only 2C/4T and may […]
Maybe soc_has_reduced_io or soc_is_reduced_io_sku
https://review.coreboot.org/c/coreboot/+/42833/1/src/soc/amd/picasso/soc_uti... PS1, Line 214: SILICON_RV2
it would be equivalent, but i'm not sure if i should use the functions here that are mostly there fo […]
I prefer improved readability whenever possible. I don't know about the CPUID issues.
Hello build bot (Jenkins), Angel Pons, Rob Barnes,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42833
to look at the new patch set (#3).
Change subject: soc/amd/picasso/soc_util: rework reduced I/O chip detection ......................................................................
soc/amd/picasso/soc_util: rework reduced I/O chip detection
Both Dali and Pollock chips have less PCIe, USB3 and Displayport connectivity. While Dali can either be fused-down PCO or RV2 silicon, Pollock is always RV2 silicon.
Since we have all boards using this code in tree right now, soc_is_dali() can be renamed and generalized to soc_is_reduced_io_sku().
Change-Id: I9eb57595da6f806305552128b0c077ceeb7c4661 Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/mainboard/amd/mandolin/mainboard.c M src/mainboard/google/zork/variants/baseboard/fsps_baseboard_trembyle.c M src/soc/amd/picasso/include/soc/soc_util.h M src/soc/amd/picasso/soc_util.c 4 files changed, 6 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/42833/3
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42833 )
Change subject: soc/amd/picasso/soc_util: rework reduced I/O chip detection ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42833/1/src/soc/amd/picasso/soc_uti... File src/soc/amd/picasso/soc_util.c:
https://review.coreboot.org/c/coreboot/+/42833/1/src/soc/amd/picasso/soc_uti... PS1, Line 212: bool soc_is_defeatured_sku(void)
Maybe soc_has_reduced_io or soc_is_reduced_io_sku
Done, used soc_is_reduced_io_sku
https://review.coreboot.org/c/coreboot/+/42833/1/src/soc/amd/picasso/soc_uti... PS1, Line 214: SILICON_RV2
I prefer improved readability whenever possible. I don't know about the CPUID issues.
the change took a bit of a different turn as i expected, but i got rid of soc_is_dali and reworked that into soc_is_reduced_io_sku. not 100% sure if this resolves this for you though
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42833 )
Change subject: soc/amd/picasso/soc_util: rework reduced I/O chip detection ......................................................................
Patch Set 3: Code-Review+1
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42833 )
Change subject: soc/amd/picasso/soc_util: rework reduced I/O chip detection ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42833/1/src/soc/amd/picasso/soc_uti... File src/soc/amd/picasso/soc_util.c:
https://review.coreboot.org/c/coreboot/+/42833/1/src/soc/amd/picasso/soc_uti... PS1, Line 214: SILICON_RV2
the change took a bit of a different turn as i expected, but i got rid of soc_is_dali and reworked t […]
LGTM
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42833 )
Change subject: soc/amd/picasso/soc_util: rework reduced I/O chip detection ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42833/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42833/1//COMMIT_MSG@8 PS1, Line 8:
nit: maybe mention what makes a SKU defeatured?
Done. The patch took a bit different route than planned, but I'd say that I've addressed this in the new commit message
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42833 )
Change subject: soc/amd/picasso/soc_util: rework reduced I/O chip detection ......................................................................
Patch Set 4: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/42833/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42833/4//COMMIT_MSG@9 PS4, Line 9: Displayport DisplayPort
Hello build bot (Jenkins), Angel Pons, Rob Barnes,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42833
to look at the new patch set (#5).
Change subject: soc/amd/picasso/soc_util: rework reduced I/O chip detection ......................................................................
soc/amd/picasso/soc_util: rework reduced I/O chip detection
Both Dali and Pollock chips have less PCIe, USB3 and DisplayPort connectivity. While Dali can either be fused-down PCO or RV2 silicon, Pollock is always RV2 silicon.
Since we have all boards using this code in tree right now, soc_is_dali() can be renamed and generalized to soc_is_reduced_io_sku().
Change-Id: I9eb57595da6f806305552128b0c077ceeb7c4661 Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/mainboard/amd/mandolin/mainboard.c M src/mainboard/google/zork/variants/baseboard/fsps_baseboard_trembyle.c M src/soc/amd/picasso/include/soc/soc_util.h M src/soc/amd/picasso/soc_util.c 4 files changed, 6 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/42833/5
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42833 )
Change subject: soc/amd/picasso/soc_util: rework reduced I/O chip detection ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42833/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42833/4//COMMIT_MSG@9 PS4, Line 9: Displayport
DisplayPort
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42833 )
Change subject: soc/amd/picasso/soc_util: rework reduced I/O chip detection ......................................................................
Patch Set 5: Code-Review+2
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42833 )
Change subject: soc/amd/picasso/soc_util: rework reduced I/O chip detection ......................................................................
soc/amd/picasso/soc_util: rework reduced I/O chip detection
Both Dali and Pollock chips have less PCIe, USB3 and DisplayPort connectivity. While Dali can either be fused-down PCO or RV2 silicon, Pollock is always RV2 silicon.
Since we have all boards using this code in tree right now, soc_is_dali() can be renamed and generalized to soc_is_reduced_io_sku().
Change-Id: I9eb57595da6f806305552128b0c077ceeb7c4661 Signed-off-by: Felix Held felix-coreboot@felixheld.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/42833 Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Rob Barnes robbarnes@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/amd/mandolin/mainboard.c M src/mainboard/google/zork/variants/baseboard/fsps_baseboard_trembyle.c M src/soc/amd/picasso/include/soc/soc_util.h M src/soc/amd/picasso/soc_util.c 4 files changed, 6 insertions(+), 6 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved Rob Barnes: Looks good to me, but someone else must approve
diff --git a/src/mainboard/amd/mandolin/mainboard.c b/src/mainboard/amd/mandolin/mainboard.c index 2e66c65..54f5775 100644 --- a/src/mainboard/amd/mandolin/mainboard.c +++ b/src/mainboard/amd/mandolin/mainboard.c @@ -310,7 +310,7 @@ const fsp_ddi_descriptor **ddi_descs, size_t *ddi_num) { /* Dali */ - if (soc_is_dali()) { + if (soc_is_reduced_io_sku()) { *pcie_descs = dali_pcie_descriptors; *pcie_num = ARRAY_SIZE(dali_pcie_descriptors); *ddi_descs = dali_ddi_descriptors; diff --git a/src/mainboard/google/zork/variants/baseboard/fsps_baseboard_trembyle.c b/src/mainboard/google/zork/variants/baseboard/fsps_baseboard_trembyle.c index f5f4842..df42f6b 100644 --- a/src/mainboard/google/zork/variants/baseboard/fsps_baseboard_trembyle.c +++ b/src/mainboard/google/zork/variants/baseboard/fsps_baseboard_trembyle.c @@ -114,7 +114,7 @@ const fsp_pcie_descriptor *baseboard_get_pcie_descriptors(size_t *num) { /* Type 2 or Type 1 fused like Type 2. */ - if (soc_is_dali()) { + if (soc_is_reduced_io_sku()) { *num = ARRAY_SIZE(dali_pcie_descriptors); return dali_pcie_descriptors; } else { @@ -176,7 +176,7 @@ const fsp_ddi_descriptor *baseboard_get_ddi_descriptors(size_t *num) { /* Type 2 or Type 1 fused like Type 2. */ - if (soc_is_dali()) { + if (soc_is_reduced_io_sku()) { *num = ARRAY_SIZE(dali_ddi_descriptors); return dali_ddi_descriptors; } else { diff --git a/src/soc/amd/picasso/include/soc/soc_util.h b/src/soc/amd/picasso/include/soc/soc_util.h index 2459207..0de0643 100644 --- a/src/soc/amd/picasso/include/soc/soc_util.h +++ b/src/soc/amd/picasso/include/soc/soc_util.h @@ -34,7 +34,7 @@ void print_soc_type(void);
/* function to determine the connectivity feature set */ -bool soc_is_dali(void); +bool soc_is_reduced_io_sku(void);
/* function to determine the iGPU type */ bool soc_is_raven2(void); diff --git a/src/soc/amd/picasso/soc_util.c b/src/soc/amd/picasso/soc_util.c index 2aa9daa..64a97a0 100644 --- a/src/soc/amd/picasso/soc_util.c +++ b/src/soc/amd/picasso/soc_util.c @@ -199,9 +199,9 @@ } }
-bool soc_is_dali(void) +bool soc_is_reduced_io_sku(void) { - return get_soc_type() == SOC_DALI; + return get_silicon_type() == SILICON_RV2 || get_soc_type() == SOC_DALI; }
bool soc_is_raven2(void)
-
Angel Pons (Code Review) -
Felix Held (Code Review)
-
Paul Menzel (Code Review)
-
Rob Barnes (Code Review)