Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39613 )
Change subject: sc7180: Add display 10nm phy & pll programming support ......................................................................
Patch Set 34: Code-Review+2
(15 comments)
https://review.coreboot.org/c/coreboot/+/39613/17/src/soc/qualcomm/sc7180/di... File src/soc/qualcomm/sc7180/display/dsi_phy.c:
https://review.coreboot.org/c/coreboot/+/39613/17/src/soc/qualcomm/sc7180/di... PS17, Line 14: */
The project uses SPDX headers now.
Done
https://review.coreboot.org/c/coreboot/+/39613/17/src/soc/qualcomm/sc7180/di... PS17, Line 30: HAL_DSI_PHY_PLL_READY_TIMEOUT
Please add the unit into the macro name.
Done
https://review.coreboot.org/c/coreboot/+/39613/17/src/soc/qualcomm/sc7180/di... PS17, Line 45: { 2, 11},
Please add a space before the closing }.
Done
https://review.coreboot.org/c/coreboot/+/39613/17/src/soc/qualcomm/sc7180/di... PS17, Line 81: */
Please follow https://doc.coreboot.org/coding_style.html#commenting.
Done
https://review.coreboot.org/c/coreboot/+/39613/23/src/soc/qualcomm/sc7180/di... File src/soc/qualcomm/sc7180/display/dsi_phy.c:
https://review.coreboot.org/c/coreboot/+/39613/23/src/soc/qualcomm/sc7180/di... PS23, Line 22: S_DIV_ROUND_UP
src/commonlib/bsd/include/commonlib/bsd/helpers. […]
This is a signed round-up, while the one in helpers.h is unsigned.
https://review.coreboot.org/c/coreboot/+/39613/32/src/soc/qualcomm/sc7180/di... File src/soc/qualcomm/sc7180/display/dsi_phy.c:
https://review.coreboot.org/c/coreboot/+/39613/32/src/soc/qualcomm/sc7180/di... PS32, Line 39: static struct dsi_phy_divider_lut_entry_type pll_dividerlut_dphy[] = {
If there's an assumption that pll_post_div must always be a power of 2, that should be documented he […]
Done
https://review.coreboot.org/c/coreboot/+/39613/32/src/soc/qualcomm/sc7180/di... PS32, Line 79: uint32_t escape_freq;
unused?
Done
https://review.coreboot.org/c/coreboot/+/39613/32/src/soc/qualcomm/sc7180/di... PS32, Line 622: for (i = 0; i < 4; i++) {
I think you can replace this loop with just log2(pll_cfg->pll_post_div)?
Done
https://review.coreboot.org/c/coreboot/+/39613/32/src/soc/qualcomm/sc7180/di... PS32, Line 739: while (i < ARRAY_SIZE(clks)) {
nit: This should really use for, not while.
Done
https://review.coreboot.org/c/coreboot/+/39613/34/src/soc/qualcomm/sc7180/di... File src/soc/qualcomm/sc7180/display/dsi_phy.c:
https://review.coreboot.org/c/coreboot/+/39613/34/src/soc/qualcomm/sc7180/di... PS34, Line 728: for (i=0; i < ARRAY_SIZE(clks); i++) {
spaces required around that '=' (ctx:VxV)
Please still address this one.
https://review.coreboot.org/c/coreboot/+/39613/32/src/soc/qualcomm/sc7180/di... File src/soc/qualcomm/sc7180/display/dsi_phy_pll.c:
https://review.coreboot.org/c/coreboot/+/39613/32/src/soc/qualcomm/sc7180/di... PS32, Line 21: u32 pll_lockdet_rate;
unused?
Done
https://review.coreboot.org/c/coreboot/+/39613/32/src/soc/qualcomm/sc7180/di... PS32, Line 93: regs->pll_prop_gain_rate = 12;
Where is this used?
Done
https://review.coreboot.org/c/coreboot/+/39613/32/src/soc/qualcomm/sc7180/di... PS32, Line 132: 0x08
Should this maybe not be hardcoded?
Done
https://review.coreboot.org/c/coreboot/+/39613/26/src/soc/qualcomm/sc7180/in... File src/soc/qualcomm/sc7180/include/soc/display/display_resources.h:
https://review.coreboot.org/c/coreboot/+/39613/26/src/soc/qualcomm/sc7180/in... PS26, Line 22: uint32_t clk_pll_m;
mdss_clock_configure() passes m and n as arguments. […]
Ack
https://review.coreboot.org/c/coreboot/+/39613/23/src/soc/qualcomm/sc7180/in... File src/soc/qualcomm/sc7180/include/soc/display/dsi_phy_pll.h:
https://review.coreboot.org/c/coreboot/+/39613/23/src/soc/qualcomm/sc7180/in... PS23, Line 32: s64
The 's64' data type is defined in <stdint.h>, so yes, you need to include that header in this file. […]
Done