John Su has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48556 )
Change subject: soc/amd/picasso: disable FCH_48MHZ_CLK_OUT ......................................................................
soc/amd/picasso: disable FCH_48MHZ_CLK_OUT
If we determine to use external oscillator 48MH, I will disable FCH_48MHZ_CLK_OUT for vilboz.
Signed-off-by: John Su john_su@compal.corp-partner.google.com Change-Id: Ida747938373f648524b1e7f34bc69e372a69c4f9 --- M src/soc/amd/picasso/fch.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/48556/1
diff --git a/src/soc/amd/picasso/fch.c b/src/soc/amd/picasso/fch.c index cda509b..d55bd36 100644 --- a/src/soc/amd/picasso/fch.c +++ b/src/soc/amd/picasso/fch.c @@ -91,7 +91,7 @@ u32 ctrl;
ctrl = misc_read32(MISC_CLK_CNTL1); - ctrl |= BP_X48M0_OUTPUT_EN; + ctrl &= ~BP_X48M0_OUTPUT_EN; misc_write32(MISC_CLK_CNTL1, ctrl); }
John Su has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48556 )
Change subject: mb/google/zork/var/vilboz: disable FCH_48MHZ_CLK_OUT for LTE sku ......................................................................
Patch Set 4:
This change is ready for review.
John Su has removed Jason Glenesk from this change. ( https://review.coreboot.org/c/coreboot/+/48556 )
Change subject: mb/google/zork/var/vilboz: disable FCH_48MHZ_CLK_OUT for LTE sku ......................................................................
Removed reviewer Jason Glenesk.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48556 )
Change subject: mb/google/zork/var/vilboz: disable FCH_48MHZ_CLK_OUT for LTE sku ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/48556/4/src/mainboard/google/zork/m... File src/mainboard/google/zork/mainboard.c:
https://review.coreboot.org/c/coreboot/+/48556/4/src/mainboard/google/zork/m... PS4, Line 244: if (board_id() > 2) move to board level.
https://review.coreboot.org/c/coreboot/+/48556/4/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h:
https://review.coreboot.org/c/coreboot/+/48556/4/src/mainboard/google/zork/v... PS4, Line 84: /* Disable FCH 48MHz clock for LTE SKU. */ add more comment like :for vilboz LTE sku.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48556 )
Change subject: mb/google/zork/var/vilboz: disable FCH_48MHZ_CLK_OUT for LTE sku ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48556/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48556/4//COMMIT_MSG@13 PS4, Line 13: add TEST here
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48556 )
Change subject: mb/google/zork/var/vilboz: disable FCH_48MHZ_CLK_OUT for LTE sku ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48556/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48556/4//COMMIT_MSG@9 PS4, Line 9: Disable FCH_48MHZ_CLK_OUT for LTE sku in next build. Could you add explanation why 48MHz clock output needs to enabled from reset to end of ramstage? With no access to schema, I am assuming pin would not be routed to anything and should never be enabled for reference clock in the first place.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48556 )
Change subject: mb/google/zork/var/vilboz: disable FCH_48MHZ_CLK_OUT for LTE sku ......................................................................
Patch Set 4:
have you verified that this won't impact audio? this gets enabled in the acp initialization and it has some comment that this is internally routed to the I2S block
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48556 )
Change subject: mb/google/zork/var/vilboz: disable FCH_48MHZ_CLK_OUT for LTE sku ......................................................................
Patch Set 4:
(2 comments)
Patch Set 4:
have you verified that this won't impact audio? this gets enabled in the acp initialization and it has some comment that this is internally routed to the I2S block
LTE sku will have external clock source chip. Please refer the issue.
https://review.coreboot.org/c/coreboot/+/48556/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48556/4//COMMIT_MSG@9 PS4, Line 9: Disable FCH_48MHZ_CLK_OUT for LTE sku in next build.
Could you add explanation why 48MHz clock output needs to enabled from reset to end of ramstage? Wit […]
Will try mainboard_init as well. Not sure this value will be override by FSP. If not, I think mainboard_init is fine.
https://review.coreboot.org/c/coreboot/+/48556/4/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h:
https://review.coreboot.org/c/coreboot/+/48556/4/src/mainboard/google/zork/v... PS4, Line 84: /* Disable FCH 48MHz clock for LTE SKU. */
add more comment like :for vilboz LTE sku.
change function name to variant_mainboard_final
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48556 )
Change subject: mb/google/zork/var/vilboz: disable FCH_48MHZ_CLK_OUT for LTE sku ......................................................................
Patch Set 4: Code-Review-1
(2 comments)
https://review.coreboot.org/c/coreboot/+/48556/4/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/vilboz/variant.c:
https://review.coreboot.org/c/coreboot/+/48556/4/src/mainboard/google/zork/v... PS4, Line 49: sku_id Please do not use SKU IDs for feature differentiation. If this is required a new FW_CONFIG bit should be added.
https://review.coreboot.org/c/coreboot/+/48556/4/src/mainboard/google/zork/v... PS4, Line 53: MISC_CLK_CNTL1 This register should not be touched directly by the mainboard. Instead there should be a chip config added to `struct soc_amd_picasso_config`: https://review.coreboot.org/cgit/coreboot.git/tree/src/soc/amd/picasso/chip....
and mainboard should just set/clear that config. Rest of the work should be done as part of SoC code.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48556 )
Change subject: mb/google/zork/var/vilboz: disable FCH_48MHZ_CLK_OUT for LTE sku ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48556/4/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/vilboz/variant.c:
https://review.coreboot.org/c/coreboot/+/48556/4/src/mainboard/google/zork/v... PS4, Line 8: vilboz_lte_sku Why are LTE SKUs being identified using SKU ID#? This is not correct. Please use a FW_CONFIG (HAS_LTE or something similar) to identify this.
John Su has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48556 )
Change subject: mb/google/zork/var/vilboz: disable FCH_48MHZ_CLK_OUT for LTE sku ......................................................................
Patch Set 6:
This change is ready for review.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48556 )
Change subject: mb/google/zork/var/vilboz: disable FCH_48MHZ_CLK_OUT for LTE sku ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/48556/6/src/mainboard/google/zork/m... File src/mainboard/google/zork/mainboard.c:
https://review.coreboot.org/c/coreboot/+/48556/6/src/mainboard/google/zork/m... PS6, Line 165: FCH_48MHZ_CLK_OUT_INIT(); change function name here and comment..
https://review.coreboot.org/c/coreboot/+/48556/6/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h:
https://review.coreboot.org/c/coreboot/+/48556/6/src/mainboard/google/zork/v... PS6, Line 86: /* Check WWAN to init mainboard. */ change comment like "Do variant mainboard init"
https://review.coreboot.org/c/coreboot/+/48556/6/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/vilboz/variant.c:
https://review.coreboot.org/c/coreboot/+/48556/6/src/mainboard/google/zork/v... PS6, Line 41: soc_cfg->bp_x480m_disable = 1; rebase to the latest patch
Hello build bot (Jenkins), Furquan Shaikh, Martin Roth, Marshall Dawson, Kangheui Won, Eric Peers, Rob Barnes, chris wang, EricR Lai, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48556
to look at the new patch set (#7).
Change subject: mb/google/zork/var/vilboz: disable FCH_48MHZ_CLK_OUT for LTE sku ......................................................................
mb/google/zork/var/vilboz: disable FCH_48MHZ_CLK_OUT for LTE sku
Disable FCH_48MHZ_CLK_OUT for LTE sku in next build.
BUG=b:174121847 BRANCH=zork TEST=build vilboz and check MISC_CLK_CNTL1.
Signed-off-by: John Su john_su@compal.corp-partner.google.com Change-Id: Ida747938373f648524b1e7f34bc69e372a69c4f9 --- M src/mainboard/google/zork/mainboard.c M src/mainboard/google/zork/variants/baseboard/helpers.c M src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/zork/variants/vilboz/variant.c 4 files changed, 28 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/48556/7
John Su has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48556 )
Change subject: mb/google/zork/var/vilboz: disable FCH_48MHZ_CLK_OUT for LTE sku ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/c/coreboot/+/48556/6/src/mainboard/google/zork/m... File src/mainboard/google/zork/mainboard.c:
https://review.coreboot.org/c/coreboot/+/48556/6/src/mainboard/google/zork/m... PS6, Line 165: FCH_48MHZ_CLK_OUT_INIT();
change function name here and comment..
Done
https://review.coreboot.org/c/coreboot/+/48556/6/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h:
https://review.coreboot.org/c/coreboot/+/48556/6/src/mainboard/google/zork/v... PS6, Line 86: /* Check WWAN to init mainboard. */
change comment like "Do variant mainboard init"
Done
https://review.coreboot.org/c/coreboot/+/48556/6/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/vilboz/variant.c:
https://review.coreboot.org/c/coreboot/+/48556/6/src/mainboard/google/zork/v... PS6, Line 41: soc_cfg->bp_x480m_disable = 1;
rebase to the latest patch
Done
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48556 )
Change subject: mb/google/zork/var/vilboz: disable FCH_48MHZ_CLK_OUT for LTE sku ......................................................................
Patch Set 7:
Wait for FW config final. Should change the comment like WWAN sku will use external clock source at next build...etc. Title can change to enable use_external_48MHz_osc
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48556 )
Change subject: mb/google/zork/var/vilboz: disable FCH_48MHZ_CLK_OUT for LTE sku ......................................................................
Patch Set 7: Code-Review+1
Hello build bot (Jenkins), Furquan Shaikh, Martin Roth, Marshall Dawson, Kangheui Won, Eric Peers, Rob Barnes, chris wang, EricR Lai, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48556
to look at the new patch set (#8).
Change subject: mb/google/zork/var/vilboz: enable use_external_48MHz_osc ......................................................................
mb/google/zork/var/vilboz: enable use_external_48MHz_osc
The vilboz WWAN sku will use external clock source at next build. Add flag to disable BP_X48M0_OUTPUT_EN or not.
BUG=b:174121847 BRANCH=zork TEST=build vilboz and check MISC_CLK_CNTL1.
Signed-off-by: John Su john_su@compal.corp-partner.google.com Change-Id: Ida747938373f648524b1e7f34bc69e372a69c4f9 --- M src/mainboard/google/zork/mainboard.c M src/mainboard/google/zork/variants/baseboard/helpers.c M src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/zork/variants/vilboz/variant.c 4 files changed, 28 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/48556/8
Hello build bot (Jenkins), Furquan Shaikh, Martin Roth, Marshall Dawson, Kangheui Won, Eric Peers, Rob Barnes, chris wang, EricR Lai, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48556
to look at the new patch set (#9).
Change subject: mb/google/zork/var/vilboz: enable use_external_48MHz_osc ......................................................................
mb/google/zork/var/vilboz: enable use_external_48MHz_osc
The vilboz WWAN sku will use external clock source at next build. Add flag to disable BP_X48M0_OUTPUT_EN.
BUG=b:174121847 BRANCH=zork TEST=build vilboz and check MISC_CLK_CNTL1.
Signed-off-by: John Su john_su@compal.corp-partner.google.com Change-Id: Ida747938373f648524b1e7f34bc69e372a69c4f9 --- M src/mainboard/google/zork/mainboard.c M src/mainboard/google/zork/variants/baseboard/helpers.c M src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/zork/variants/vilboz/variant.c 4 files changed, 28 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/48556/9
John Su has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48556 )
Change subject: mb/google/zork/var/vilboz: enable use_external_48MHz_osc ......................................................................
Patch Set 9:
Patch Set 7:
Wait for FW config final. Should change the comment like WWAN sku will use external clock source at next build...etc. Title can change to enable use_external_48MHz_osc
John Su has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48556 )
Change subject: mb/google/zork/var/vilboz: enable use_external_48MHz_osc ......................................................................
Patch Set 9:
(9 comments)
https://review.coreboot.org/c/coreboot/+/48556/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48556/4//COMMIT_MSG@9 PS4, Line 9: Disable FCH_48MHZ_CLK_OUT for LTE sku in next build.
Will try mainboard_init as well. Not sure this value will be override by FSP. […]
Done
https://review.coreboot.org/c/coreboot/+/48556/4//COMMIT_MSG@13 PS4, Line 13:
add TEST here
Done
https://review.coreboot.org/c/coreboot/+/48556/4/src/mainboard/google/zork/m... File src/mainboard/google/zork/mainboard.c:
https://review.coreboot.org/c/coreboot/+/48556/4/src/mainboard/google/zork/m... PS4, Line 244: if (board_id() > 2)
move to board level.
Done
https://review.coreboot.org/c/coreboot/+/48556/5/src/mainboard/google/zork/m... File src/mainboard/google/zork/mainboard.c:
https://review.coreboot.org/c/coreboot/+/48556/5/src/mainboard/google/zork/m... PS5, Line 165: FCH_48MHZ_CLK_OUT_INIT();
variant_mainboard_init
Done
https://review.coreboot.org/c/coreboot/+/48556/4/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h:
https://review.coreboot.org/c/coreboot/+/48556/4/src/mainboard/google/zork/v... PS4, Line 84: /* Disable FCH 48MHz clock for LTE SKU. */
change function name to variant_mainboard_final
Done
https://review.coreboot.org/c/coreboot/+/48556/4/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/vilboz/variant.c:
https://review.coreboot.org/c/coreboot/+/48556/4/src/mainboard/google/zork/v... PS4, Line 8: vilboz_lte_sku
It would be good to start an issue for this request.
Done
https://review.coreboot.org/c/coreboot/+/48556/4/src/mainboard/google/zork/v... PS4, Line 49: sku_id
Please do not use SKU IDs for feature differentiation. […]
Done
https://review.coreboot.org/c/coreboot/+/48556/4/src/mainboard/google/zork/v... PS4, Line 53: MISC_CLK_CNTL1
This register should not be touched directly by the mainboard. […]
Done
https://review.coreboot.org/c/coreboot/+/48556/1/src/soc/amd/picasso/fch.c File src/soc/amd/picasso/fch.c:
https://review.coreboot.org/c/coreboot/+/48556/1/src/soc/amd/picasso/fch.c@9... PS1, Line 94: ctrl &= ~BP_X48M0_OUTPUT_EN;
need move to board level.
Done
John Su has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48556 )
Change subject: mb/google/zork/var/vilboz: enable use_external_48MHz_osc ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48556/5/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/helpers.c:
https://review.coreboot.org/c/coreboot/+/48556/5/src/mainboard/google/zork/v... PS5, Line 189: void __weak FCH_48MHZ_CLK_OUT_INIT(void)
same above
Done
John Su has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48556 )
Change subject: mb/google/zork/var/vilboz: enable use_external_48MHz_osc ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48556/5/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h:
https://review.coreboot.org/c/coreboot/+/48556/5/src/mainboard/google/zork/v... PS5, Line 87: void FCH_48MHZ_CLK_OUT_INIT(void);
same
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48556 )
Change subject: mb/google/zork/var/vilboz: enable use_external_48MHz_osc ......................................................................
Patch Set 9:
(3 comments)
https://review.coreboot.org/c/coreboot/+/48556/9/src/mainboard/google/zork/m... File src/mainboard/google/zork/mainboard.c:
https://review.coreboot.org/c/coreboot/+/48556/9/src/mainboard/google/zork/m... PS9, Line 165: variant_mainboard_init variant_devtree_update() already exists to allow this. You don't need another callback. All you need is to implement variant_devtree_update for vilboz.
https://review.coreboot.org/c/coreboot/+/48556/9/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/vilboz/variant.c:
https://review.coreboot.org/c/coreboot/+/48556/9/src/mainboard/google/zork/v... PS9, Line 39: A comment here would be very helpful explaining why this config is set for LTE/WWAN variants.
https://review.coreboot.org/c/coreboot/+/48556/9/src/mainboard/google/zork/v... PS9, Line 40: if (variant_has_wwan()) Don't you also need to check board_id since the external OSC is not supported on previous builds?
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48556 )
Change subject: mb/google/zork/var/vilboz: enable use_external_48MHz_osc ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48556/9/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/vilboz/variant.c:
https://review.coreboot.org/c/coreboot/+/48556/9/src/mainboard/google/zork/v... PS9, Line 40: if (variant_has_wwan())
Don't you also need to check board_id since the external OSC is not supported on previous builds?
This is my question as well. Because previous builds DUT didn't have this FW config field, so this would never be true right?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48556 )
Change subject: mb/google/zork/var/vilboz: enable use_external_48MHz_osc ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48556/9/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/vilboz/variant.c:
https://review.coreboot.org/c/coreboot/+/48556/9/src/mainboard/google/zork/v... PS9, Line 40: if (variant_has_wwan())
This is my question as well. […]
Interesting question. You are right that older boards should have this bit set to 0, so it should be safe.
Hello build bot (Jenkins), Furquan Shaikh, Martin Roth, Marshall Dawson, Kangheui Won, Bhanu Prakash Maiya, Eric Peers, Rob Barnes, chris wang, EricR Lai, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48556
to look at the new patch set (#10).
Change subject: mb/google/zork/var/vilboz: disable FCH_48MHZ_CLK_OUT for LTE sku ......................................................................
mb/google/zork/var/vilboz: disable FCH_48MHZ_CLK_OUT for LTE sku
Disable FCH_48MHZ_CLK_OUT for LTE sku in next build.
BUG=b:174121847 BRANCH=zork TEST=build vilboz and check MISC_CLK_CNTL1.
Signed-off-by: John Su john_su@compal.corp-partner.google.com Change-Id: Ida747938373f648524b1e7f34bc69e372a69c4f9 --- M src/mainboard/google/zork/mainboard.c M src/mainboard/google/zork/variants/baseboard/helpers.c M src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/zork/variants/vilboz/variant.c 4 files changed, 21 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/48556/10
Hello build bot (Jenkins), Furquan Shaikh, Martin Roth, Marshall Dawson, Kangheui Won, Bhanu Prakash Maiya, Eric Peers, Rob Barnes, chris wang, EricR Lai, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48556
to look at the new patch set (#11).
Change subject: mb/google/zork/var/vilboz: disable FCH_48MHZ_CLK_OUT for LTE sku ......................................................................
mb/google/zork/var/vilboz: disable FCH_48MHZ_CLK_OUT for LTE sku
Disable FCH_48MHZ_CLK_OUT for LTE sku in next build.
BUG=b:174121847 BRANCH=zork TEST=build vilboz and check MISC_CLK_CNTL1.
Signed-off-by: John Su john_su@compal.corp-partner.google.com Change-Id: Ida747938373f648524b1e7f34bc69e372a69c4f9 --- M src/mainboard/google/zork/variants/baseboard/helpers.c M src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/zork/variants/vilboz/variant.c 3 files changed, 20 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/48556/11
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48556 )
Change subject: mb/google/zork/var/vilboz: disable FCH_48MHZ_CLK_OUT for LTE sku ......................................................................
Patch Set 10: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/48556/10/src/mainboard/google/zork/... File src/mainboard/google/zork/mainboard.c:
https://review.coreboot.org/c/coreboot/+/48556/10/src/mainboard/google/zork/... PS10, Line 163: remove the extra line
https://review.coreboot.org/c/coreboot/+/48556/10/src/mainboard/google/zork/... File src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h:
https://review.coreboot.org/c/coreboot/+/48556/10/src/mainboard/google/zork/... PS10, Line 84: not necessary change.
Hello build bot (Jenkins), Furquan Shaikh, Martin Roth, Marshall Dawson, Kangheui Won, Bhanu Prakash Maiya, Eric Peers, Rob Barnes, chris wang, EricR Lai, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48556
to look at the new patch set (#12).
Change subject: mb/google/zork/var/vilboz: Add enable use_external_48MHz_osc flag ......................................................................
mb/google/zork/var/vilboz: Add enable use_external_48MHz_osc flag
Add enable use_external_48MHz_osc flag and then WWAN sku will use external clock source at next build.
BUG=b:174121847 BRANCH=zork TEST=build vilboz and check MISC_CLK_CNTL1.
Signed-off-by: John Su john_su@compal.corp-partner.google.com Change-Id: Ida747938373f648524b1e7f34bc69e372a69c4f9 --- M src/mainboard/google/zork/variants/baseboard/helpers.c M src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/zork/variants/vilboz/variant.c 3 files changed, 20 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/48556/12
John Su has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48556 )
Change subject: mb/google/zork/var/vilboz: Add enable use_external_48MHz_osc flag ......................................................................
Patch Set 12:
(5 comments)
https://review.coreboot.org/c/coreboot/+/48556/9/src/mainboard/google/zork/m... File src/mainboard/google/zork/mainboard.c:
https://review.coreboot.org/c/coreboot/+/48556/9/src/mainboard/google/zork/m... PS9, Line 165: variant_mainboard_init
variant_devtree_update() already exists to allow this. You don't need another callback. […]
Done
https://review.coreboot.org/c/coreboot/+/48556/10/src/mainboard/google/zork/... File src/mainboard/google/zork/mainboard.c:
https://review.coreboot.org/c/coreboot/+/48556/10/src/mainboard/google/zork/... PS10, Line 163:
remove the extra line
Done
https://review.coreboot.org/c/coreboot/+/48556/10/src/mainboard/google/zork/... File src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h:
https://review.coreboot.org/c/coreboot/+/48556/10/src/mainboard/google/zork/... PS10, Line 84:
not necessary change.
Done
https://review.coreboot.org/c/coreboot/+/48556/9/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/vilboz/variant.c:
https://review.coreboot.org/c/coreboot/+/48556/9/src/mainboard/google/zork/v... PS9, Line 39:
A comment here would be very helpful explaining why this config is set for LTE/WWAN variants.
Done
https://review.coreboot.org/c/coreboot/+/48556/9/src/mainboard/google/zork/v... PS9, Line 40: if (variant_has_wwan())
Interesting question. […]
Done
John Su has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48556 )
Change subject: mb/google/zork/var/vilboz: Add enable use_external_48MHz_osc flag ......................................................................
Patch Set 13:
This change is ready for review.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48556 )
Change subject: mb/google/zork/var/vilboz: Add enable use_external_48MHz_osc flag ......................................................................
Patch Set 13: Code-Review+2
nit: please change comment as well due to flag name change.
Hello build bot (Jenkins), Furquan Shaikh, Martin Roth, Marshall Dawson, Kangheui Won, Bhanu Prakash Maiya, Eric Peers, Rob Barnes, chris wang, EricR Lai, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48556
to look at the new patch set (#14).
Change subject: mb/google/zork/var/vilboz: Add enable acp_i2s_use_external_48mhz_osc flag ......................................................................
mb/google/zork/var/vilboz: Add enable acp_i2s_use_external_48mhz_osc flag
Add enable acp_i2s_use_external_48mhz_osc flag and then WWAN sku will use external clock source at next build.
BUG=b:174121847 BRANCH=zork TEST=build vilboz and check MISC_CLK_CNTL1.
Signed-off-by: John Su john_su@compal.corp-partner.google.com Change-Id: Ida747938373f648524b1e7f34bc69e372a69c4f9 --- M src/mainboard/google/zork/variants/baseboard/helpers.c M src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/zork/variants/vilboz/variant.c 3 files changed, 20 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/48556/14
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48556 )
Change subject: mb/google/zork/var/vilboz: Add enable acp_i2s_use_external_48mhz_osc flag ......................................................................
Patch Set 14: Code-Review+2
Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48556 )
Change subject: mb/google/zork/var/vilboz: Add enable acp_i2s_use_external_48mhz_osc flag ......................................................................
mb/google/zork/var/vilboz: Add enable acp_i2s_use_external_48mhz_osc flag
Add enable acp_i2s_use_external_48mhz_osc flag and then WWAN sku will use external clock source at next build.
BUG=b:174121847 BRANCH=zork TEST=build vilboz and check MISC_CLK_CNTL1.
Signed-off-by: John Su john_su@compal.corp-partner.google.com Change-Id: Ida747938373f648524b1e7f34bc69e372a69c4f9 Reviewed-on: https://review.coreboot.org/c/coreboot/+/48556 Reviewed-by: Felix Held felix-coreboot@felixheld.de Reviewed-by: EricR Lai ericr_lai@compal.corp-partner.google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/google/zork/variants/baseboard/helpers.c M src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/zork/variants/vilboz/variant.c 3 files changed, 20 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Felix Held: Looks good to me, approved EricR Lai: Looks good to me, approved
diff --git a/src/mainboard/google/zork/variants/baseboard/helpers.c b/src/mainboard/google/zork/variants/baseboard/helpers.c index d12a8ed4..bea0887 100644 --- a/src/mainboard/google/zork/variants/baseboard/helpers.c +++ b/src/mainboard/google/zork/variants/baseboard/helpers.c @@ -46,6 +46,9 @@ /* Fan information */ FW_CONFIG_MASK_FAN = 0x3, FW_CONFIG_SHIFT_FAN = 27, + /* WWAN presence */ + FW_CONFIG_MASK_WWAN = 0x1, + FW_CONFIG_SHIFT_WWAN = 28, };
static int get_fw_config(uint64_t *val) @@ -88,6 +91,11 @@ return !!extract_field(FW_CONFIG_MASK_NVME, FW_CONFIG_SHIFT_NVME); }
+int variant_has_wwan(void) +{ + return !!extract_field(FW_CONFIG_MASK_WWAN, FW_CONFIG_SHIFT_WWAN); +} + bool variant_uses_v3_schematics(void) { uint32_t board_version; diff --git a/src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h b/src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h index f6e7e7c..aeff49b 100644 --- a/src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h +++ b/src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h @@ -63,6 +63,8 @@ int variant_has_emmc(void); /* Return 0 if non-existent, 1 if present. */ int variant_has_nvme(void); +/* Return 0 if non-existent, 1 if present. */ +int variant_has_wwan(void);
/* Determine if booting in factory by using CROS_SKU_UNPROVISIONED. */ int boot_is_factory_unprovisioned(void); diff --git a/src/mainboard/google/zork/variants/vilboz/variant.c b/src/mainboard/google/zork/variants/vilboz/variant.c index cbc160e..fd38e1d 100644 --- a/src/mainboard/google/zork/variants/vilboz/variant.c +++ b/src/mainboard/google/zork/variants/vilboz/variant.c @@ -31,3 +31,13 @@ *ddi_descs = &hdmi_ddi_descriptors[0]; *ddi_num = ARRAY_SIZE(hdmi_ddi_descriptors); } + +void variant_devtree_update(void) +{ + struct soc_amd_picasso_config *soc_cfg; + soc_cfg = config_of_soc(); + + /* b:/174121847 Use external OSC to mitigate noise for WWAN sku. */ + if (variant_has_wwan()) + soc_cfg->acp_i2s_use_external_48mhz_osc = 1; +}
-
EricR Lai (Code Review) -
Felix Held (Code Review)
-
Furquan Shaikh (Code Review)
-
John Su (Code Review)
-
Kyösti Mälkki (Code Review)