Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31372 )
Change subject: soc/amd: Add support to soc merlinfalcon ......................................................................
Patch Set 1:
(5 comments)
This change is ready for review.
https://review.coreboot.org/#/c/31372/1/src/soc/amd/stoneyridge/include/soc/... File src/soc/amd/stoneyridge/include/soc/southbridge.h:
https://review.coreboot.org/#/c/31372/1/src/soc/amd/stoneyridge/include/soc/... PS1, Line 189: #define OSCOUT2_CLK_OUTPUT_ENB BIT(7) /* 0 = Enabled, 1 = Disabled */
This should be split out into a separate patch and applied to ST first.
ST only has 1 oscillator, it makes no sense to apply it to ST alone. This is for SOC that have more then 1 core. Stoney has 1 core with hyperthread, Merlin has 2 cores with hyperthread. It's one oscillator per core.
https://review.coreboot.org/#/c/31372/1/src/soc/amd/stoneyridge/northbridge.... File src/soc/amd/stoneyridge/northbridge.c:
https://review.coreboot.org/#/c/31372/1/src/soc/amd/stoneyridge/northbridge.... PS1, Line 341: #if IS_ENABLED(CONFIG_STONEYRIDGE_CPU_CARIZO)
No need for #if #else because this driver is to be compatible with both.
Will see how to do it, maybe following Martin's suggestion.
https://review.coreboot.org/#/c/31372/1/src/soc/amd/stoneyridge/northbridge.... PS1, Line 342: device
You'll want to change this from .device to .devices and point to a devices list.
Thanks for the tip.
https://review.coreboot.org/#/c/31372/1/src/soc/amd/stoneyridge/northbridge.... PS1, Line 460: #if IS_ENABLED(CONFIG_STONEYRIDGE_CPU_CARIZO)
Remove the #if #else like above. Just do the logic in code for both.
Again, will see how to do it.
https://review.coreboot.org/#/c/31372/1/src/soc/amd/stoneyridge/southbridge.... File src/soc/amd/stoneyridge/southbridge.c:
https://review.coreboot.org/#/c/31372/1/src/soc/amd/stoneyridge/southbridge.... PS1, Line 392: void sb_clk_output_48Mhz(u32 osc)
Split the change to this function to a separate patch (as mentioned in the .h file).
Even though it makes no sense for stoney alone?