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 26:
(21 comments)
https://review.coreboot.org/c/coreboot/+/39613/26/src/soc/qualcomm/sc7180/di... File src/soc/qualcomm/sc7180/display/dsi_phy.c:
https://review.coreboot.org/c/coreboot/+/39613/26/src/soc/qualcomm/sc7180/di... PS26, Line 92: DSI_LaneID_Max Please stick to coding style (constants in ALL_CAPS)
https://review.coreboot.org/c/coreboot/+/39613/26/src/soc/qualcomm/sc7180/di... PS26, Line 127: struct dsi_phy_configtype { Do you need all these structures and do they need all these members? Where's the difference between configtype and configtype_info? You seem to always use them together, can't they just be one structure?
A lot of the fields in here seem pointless, e.g. cphy_mode. You just hardcode it to false in mdss_dsi_phy_pll_setup() and never change it anywhere else. Please remove all these fields and just hardcode their effects directly, you should only have fields in these structures that are actually determined at runtime.
https://review.coreboot.org/c/coreboot/+/39613/26/src/soc/qualcomm/sc7180/di... PS26, Line 168: void mdss_dsi_phy_reset(void); Same as in other patches.
https://review.coreboot.org/c/coreboot/+/39613/26/src/soc/qualcomm/sc7180/di... PS26, Line 630: enum cb_err If it cannot fail, just make it void.
https://review.coreboot.org/c/coreboot/+/39613/26/src/soc/qualcomm/sc7180/di... PS26, Line 679: wmb(); /* ensure dsiclk_sel is always programmed before pll start */ write32() already contains explicit DMBs, you don't need to add explicit barriers.
https://review.coreboot.org/c/coreboot/+/39613/26/src/soc/qualcomm/sc7180/di... PS26, Line 804: uint32_t pointless cast
https://review.coreboot.org/c/coreboot/+/39613/26/src/soc/qualcomm/sc7180/di... PS26, Line 806: MAX What is this supposed to do? fval is already unsigned.
https://review.coreboot.org/c/coreboot/+/39613/26/src/soc/qualcomm/sc7180/di... PS26, Line 987: 1000 KHz
https://review.coreboot.org/c/coreboot/+/39613/26/src/soc/qualcomm/sc7180/di... PS26, Line 1034: {"disp_cc_mdss_esc0_clk", 0, 1, 0, 0, 0, 0}, Please use
.name = "disp_cc_mdss_esc0_clk", .clk_secondary_source = 1, ...
notation to clarify what each number means. (You don't need to explicitly initialize zeroes.)
https://review.coreboot.org/c/coreboot/+/39613/26/src/soc/qualcomm/sc7180/di... PS26, Line 1055: clk_enable Just put a literal 1 here, there's no point in this being a variable.
https://review.coreboot.org/c/coreboot/+/39613/26/src/soc/qualcomm/sc7180/di... PS26, Line 1058: fialed typo
https://review.coreboot.org/c/coreboot/+/39613/26/src/soc/qualcomm/sc7180/di... File src/soc/qualcomm/sc7180/display/dsi_phy_pll.c:
https://review.coreboot.org/c/coreboot/+/39613/26/src/soc/qualcomm/sc7180/di... PS26, Line 18: #define MHZ_3000 3000000000UL Please use (3000 * MHz) directly in the code rather than hardcoding constants like this.
https://review.coreboot.org/c/coreboot/+/39613/26/src/soc/qualcomm/sc7180/di... PS26, Line 20: # define do_div(n, base) ( \ Looks like you're not actually using this for what it is, so remove it (you just need a plain _tmp / __d below, not the whole remainder stuff that this macro does, and you should get rid of below macro anyway).
https://review.coreboot.org/c/coreboot/+/39613/26/src/soc/qualcomm/sc7180/di... PS26, Line 30: #define DIV_ROUND_CLOSEST_ULL(x, divisor)( \ We already have a DIV_ROUND_CLOSEST() (in src/commonlib/include/commonlib/helpers.h, chain-included from <types.h>) that should work for all data types. Just use that.
https://review.coreboot.org/c/coreboot/+/39613/26/src/soc/qualcomm/sc7180/di... PS26, Line 98: struct mdss_pll_resources *rsc unused?
https://review.coreboot.org/c/coreboot/+/39613/26/src/soc/qualcomm/sc7180/di... PS26, Line 183: 19200000 This is SRC_XO_HZ from <soc/clock.h>
https://review.coreboot.org/c/coreboot/+/39613/26/src/soc/qualcomm/sc7180/di... PS26, Line 296: 19200000 This too.
https://review.coreboot.org/c/coreboot/+/39613/26/src/soc/qualcomm/sc7180/di... PS26, Line 306: wmb(); Already done by each write32().
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 9: MDP_External_Clock_Entry Please stick to coding style (type names in lower_case).
https://review.coreboot.org/c/coreboot/+/39613/26/src/soc/qualcomm/sc7180/in... PS26, Line 22: uint32_t clk_pll_m; Looks like we never use MND for the DSI clocks we have, so do we need these fields?
https://review.coreboot.org/c/coreboot/+/39613/26/src/soc/qualcomm/sc7180/in... File src/soc/qualcomm/sc7180/include/soc/display/dsi_phy_pll.h:
https://review.coreboot.org/c/coreboot/+/39613/26/src/soc/qualcomm/sc7180/in... PS26, Line 16: struct mdss_pll_resources { This is another one of these structs that looks way larger than necessary. Please hardcode any values from here that are always the same in our case, and then remove all members that aren't used.