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 1 of 3] ......................................................................
Patch Set 2:
(16 comments)
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/dis... File src/soc/qualcomm/sc7180/display/dsi_phy.c:
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/dis... PS2, Line 204: static uint32_t dsi_phy_rounddown(SIM_DOUBLE SIM_FLOAT_value) I... don't even know what this is. If you want MAX(x, 0), then write that. And don't use weird custom integer typedefs.
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/dis... PS2, Line 649: while ((pll_status == 0) && (time_out)) { Please use
wait_us(15000, read32(...) & 1);
for wait loops like this.
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/dis... PS2, Line 1085: = Please stick to the coding style.
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/dis... PS2, Line 1087: MDPExternalClockEntry *clk_list = sDSI0ExtClocks_6xx; Same here as below: you don't need a clk_list structure if we'll only ever have one list! Just hardcode the contents of that.
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/dis... PS2, Line 1133: //Phy set up Please use /* C89 comments */ in coreboot and use the correct amount of whitespace.
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/dis... PS2, Line 1153: intf->dsi_phy_init = mdss_dsi_phy_10nm_init; Do not build abstractions that you're not using. If the only initialization function we'll ever use is mdss_dsi_phy_10nm_init, you do not need a function pointer for it, just call it where it is used and get rid of this ftable structure.
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/dis... File src/soc/qualcomm/sc7180/display/dsi_phy_pll.c:
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/dis... PS2, Line 16: #include <arch/mmio.h> Always include <device/mmio.h>, not <arch/mmio.h> directly.
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/dis... PS2, Line 227: config->disable_prescaler = false; Why are you setting up this huge data structure full of hardcoded values? Do these ever change in our case? Just hardcode them where they're used!
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/inc... File src/soc/qualcomm/sc7180/include/soc/display/display_resources.h:
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/inc... PS2, Line 24: static char byte0_intf_clk_name[MDP_MAX_CLOCK_NAME] = "disp_cc_mdss_byte0_intf_clk"; Do not use strings to identify things, make an enum or something. (I haven't read the whole code but I don't understand why things have to be so complicated here anyway. You are probably abstracting way more things than you need to, just hardcode the specifics to this SoC.)
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/inc... PS2, Line 29: typedef struct { Do not typedef structures in coreboot.
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/inc... PS2, Line 31: uint32_t uClkSource; /* Primary Clock Source */ Do not use camelCase for struct member...
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/inc... PS2, Line 38: } MDPExternalClockEntry; ...or type names (or anything else, actually), we use snake_case for those.
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/inc... File src/soc/qualcomm/sc7180/include/soc/display/msm_panel.h:
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/inc... PS2, Line 27: #define ERROR -1 Just use 0 and -1 directly in the code.
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/inc... PS2, Line 58: uint32_t v_pulse_width; For things that are in struct edid you should be using that struct, not define your own structure for the same data.
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/inc... File src/soc/qualcomm/sc7180/include/soc/mdss_6_2_0.h:
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/inc... PS2, Line 23: #define readl(a) read32((void *)(size_t)(a)) As mentioned in other CL, do not add your own definitions for these.
https://review.coreboot.org/c/coreboot/+/39613/2/src/soc/qualcomm/sc7180/inc... PS2, Line 33: #define HWIO_OUT_FLD(regVal, io, field, fldVal) \ Use struct overlays to model hardware registers and don't create your own macro infrastructure instead. If you want to model individual bit fields in registers, you can use the DEFINE_BITFIELD/SET32_BITFIELDS API in <device/mmio.h>.