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 28:
(9 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
Done
No, this is not what I meant. The whole constant name needs to be in all caps, so this should be DSI_LANEID_MAX, the one below should be DSI_LANEID_FORCE_32BIT, and all the other constants you have here should follow the same. No identifier in coreboot should ever have mixed case (other than super special exceptions like MiB), they're always either all caps or all small letters.
https://review.coreboot.org/c/coreboot/+/39613/28/src/soc/qualcomm/sc7180/di... File src/soc/qualcomm/sc7180/display/dsi_phy.c:
https://review.coreboot.org/c/coreboot/+/39613/28/src/soc/qualcomm/sc7180/di... PS28, Line 95: uint32_t decdiv_start; There are still plenty of fields (like this one) in here that aren't used in the code at all (anymore). Please check all these structures and remove all unused fields.
https://review.coreboot.org/c/coreboot/+/39613/28/src/soc/qualcomm/sc7180/di... PS28, Line 453: lane_cfg.strength_override = 0; What do you need these 6 fields for? I don't think you need struct dsi_phy_laneconfig_type at all to be honest. These are all just hardcoded here and never change. Just hardcode them where they're used.
https://review.coreboot.org/c/coreboot/+/39613/28/src/soc/qualcomm/sc7180/di... PS28, Line 495: wmb(); You don't need this wmb() either.
https://review.coreboot.org/c/coreboot/+/39613/28/src/soc/qualcomm/sc7180/di... PS28, Line 650: phy_cfg->dsi_clksel = 1; Why do you need this field?
https://review.coreboot.org/c/coreboot/+/39613/28/src/soc/qualcomm/sc7180/di... PS28, Line 760: printk(BIOS_INFO, "Desired bitclock:%u\n", (uint32_t)desired_bclk); Please add proper spacing and a unit, e.g. "Desired bitclock: %uHz"
https://review.coreboot.org/c/coreboot/+/39613/28/src/soc/qualcomm/sc7180/di... PS28, Line 780: phy_cfg.escape_freq = 19; Why do you need this field?
https://review.coreboot.org/c/coreboot/+/39613/28/src/soc/qualcomm/sc7180/di... File src/soc/qualcomm/sc7180/display/dsi_phy_pll.c:
https://review.coreboot.org/c/coreboot/+/39613/28/src/soc/qualcomm/sc7180/di... PS28, Line 34: }; A lot of unused fields in this file too (e.g. all the ssc_ stuff?). Please remove *all* unused fields from all structs!
https://review.coreboot.org/c/coreboot/+/39613/28/src/soc/qualcomm/sc7180/di... PS28, Line 59: static inline u64 div_u64(u64 dividend, u32 divisor) Why do we have these two functions, anyway? I don't really see what they add. Why write
x = div_u64(a, b);
when you could just write
x = a / b
?