Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39615 )
Change subject: sc7180: Add display hardware pipe line initialization [patch 3 of 3] ......................................................................
Patch Set 2:
(14 comments)
Sorry, this is a huge stack of code and there are some very high-level structural issues with it, I think those have to be resolved first before I can start reviewing in detail. Most of the comments below apply to many places across these patches, not just the one I'm pointing out.
https://review.coreboot.org/c/coreboot/+/39615/2/src/soc/qualcomm/sc7180/dis... File src/soc/qualcomm/sc7180/display.c:
https://review.coreboot.org/c/coreboot/+/39615/2/src/soc/qualcomm/sc7180/dis... PS2, Line 31: static struct mdss_dsi_phy_ctrl dsi_video_mode_phy_db; You should not need so many globals (in fact, ideally you shouldn't need any). Some of these look like they could just as well be a local in the respective function.
https://review.coreboot.org/c/coreboot/+/39615/2/src/soc/qualcomm/sc7180/dis... PS2, Line 32: extern int msm_display_init(struct msm_fb_panel_data *pdata); External function prototypes should go into header files.
https://review.coreboot.org/c/coreboot/+/39615/2/src/soc/qualcomm/sc7180/dis... PS2, Line 37: /* Place holder to handle dual dsi cases in future */ Don't add placeholders for things that aren't used yet, we can always add them later when we actually decide that we need them.
https://review.coreboot.org/c/coreboot/+/39615/2/src/soc/qualcomm/sc7180/dis... PS2, Line 47: * MDSS Clocks in coreboot later. If we don't need it, you don't need to implement a function or callback for it. We're only implementing one controller here, only write the code needed by that.
https://review.coreboot.org/c/coreboot/+/39615/2/src/soc/qualcomm/sc7180/dis... PS2, Line 55: // if (panelstruct.panelresetseq) //TODO: check if bridge reset seq? Don't commit commented-out code, ever.
https://review.coreboot.org/c/coreboot/+/39615/2/src/soc/qualcomm/sc7180/dis... PS2, Line 239: panel.bl_func = mdss_dsi_bl_enable; Why is all of this in function pointers?! You're only implementing a driver for MDSS DSI here, nothing else! Just hardcode the initialization flow for that.
https://review.coreboot.org/c/coreboot/+/39615/2/src/soc/qualcomm/sc7180/dis... File src/soc/qualcomm/sc7180/display/mdss.c:
https://review.coreboot.org/c/coreboot/+/39615/2/src/soc/qualcomm/sc7180/dis... PS2, Line 41: void mdp_set_revision(int rev) Do not pass information via globals like this (exporting a function that sets/reads a global also counts as passing via global). If this needs to be known by other code, it should be stored as part of a data structure that is passed between the relevant parts of the display stack. But it looks like we're always hardcoding this to 620 right now anyway, so please just take all the code making a distinction about it out and hardcode the code path that's relevant for this SoC.
https://review.coreboot.org/c/coreboot/+/39615/2/src/soc/qualcomm/sc7180/dis... PS2, Line 486: ((pinfo->dest == DISPLAY_1) && (pinfo->lcdc.pipe_swap))) { Do we need support for two destinations (I assume this is the difference between the DSI0 and DSI1 port)? Does SC7180 even *have* a DSI1 port? I don't see it on our schematics. Even if it does, we should assume that we will always hook up our displays consistently (e.g. internal panel always to DSI0) unless there are real differences between the two that could force us to use DSI1 at some point. So we should take all the code differentiating this out.
https://review.coreboot.org/c/coreboot/+/39615/2/src/soc/qualcomm/sc7180/dis... PS2, Line 653: if (pinfo->lcdc.dst_split) Do we actually ever expect to need any of these panel-specific features (SPLIT_DISPLAY, DUAL_PIPE, PIPE_SWAP, etc.)? Please explain under what circumstances we would need each of those, I have never seen another platform that required so many panel-specific configuration details. For now we should assume that we'll only need to drive eDP panels through a bridge, so we should not prematurely implement MIPI-specific features that wouldn't apply to that situation. We could still add them later if we actually got into a situation where we need them.
Please take out support for options and features that we do not expect to need to simplify this driver.
https://review.coreboot.org/c/coreboot/+/39615/2/src/soc/qualcomm/sc7180/dis... PS2, Line 678: writel The official coreboot register accessors are read32()/write32(). Do not define your own. (This is very explicitly called out in the 'firmware bring-up guide' document I have shared with and mentioned to Qualcomm on many different occasions now...)
https://review.coreboot.org/c/coreboot/+/39615/2/src/soc/qualcomm/sc7180/dis... PS2, Line 678: MDP_CTL_0_BASE + CTL_START Register accesses are supposed to be done through struct overlays. (This is also explicitly explained in that document. All the other sc7180 drivers are doing that by now.)
https://review.coreboot.org/c/coreboot/+/39615/2/src/soc/qualcomm/sc7180/dis... File src/soc/qualcomm/sc7180/display/oem_panel.c:
https://review.coreboot.org/c/coreboot/+/39615/2/src/soc/qualcomm/sc7180/dis... PS2, Line 126: int oem_panel_select(const char *panel_name, struct panel_struct *panelstruct, We do not want any hardcoded panel information on this board. These are eDP panels, they have an actual EDID we can read at runtime. Please throw all this code out and instead implement an SN65DSI86 bridge driver so you can read the EDID over the eDP AUX I2C channel. You can check out src/drivers/parade/ps8640/ps8640.c as a template (that's a similar bridge chip from a different vendor).
https://review.coreboot.org/c/coreboot/+/39615/2/src/soc/qualcomm/sc7180/inc... File src/soc/qualcomm/sc7180/include/soc/display/panel_sn65dsix6_auo_bll6xak01_dsi_video.h:
https://review.coreboot.org/c/coreboot/+/39615/2/src/soc/qualcomm/sc7180/inc... PS2, Line 231: {0x4, sn65dsix6_auo_b116xak01_dsi_video_on_cmd30, 0x10}, Do not conflate panel and bridge configuration. Bridge configuration should be done by a separate bridge-driver and should be panel agnostic. Panels should normally not need any separate initialization commands if they're eDP.
https://review.coreboot.org/c/coreboot/+/39615/2/src/soc/qualcomm/sc7180/soc... File src/soc/qualcomm/sc7180/soc.c:
https://review.coreboot.org/c/coreboot/+/39615/2/src/soc/qualcomm/sc7180/soc... PS2, Line 38: sc7180_display_init(dev); This should be kicked off from trogdor/mainboard.c, then you don't need that #if either. On the other hand, you should be checking display_init_required() before you run this. See src/mainboard/google/oak/mainboard.c for a good example (that one is also using an eDP bridge, just that it's called ps8640 instead of SN65DSI86).