Vinodkumarreddy Polimera has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39615 )
Change subject: sc7180: Add display hardware pipe line initialization ......................................................................
Patch Set 22:
(50 comments)
addressed all the comments
https://review.coreboot.org/c/coreboot/+/39615/16/src/mainboard/google/trogd... File src/mainboard/google/trogdor/mainboard.c:
https://review.coreboot.org/c/coreboot/+/39615/16/src/mainboard/google/trogd... PS16, Line 7: #include <device/i2c_simple.h> : #include <soc/display/msm_panel.h> : #include <soc/display.h> : #include <soc/qupv3_i2c.h>
please, clean up
Done
https://review.coreboot.org/c/coreboot/+/39615/17/src/mainboard/google/trogd... File src/mainboard/google/trogdor/mainboard.c:
https://review.coreboot.org/c/coreboot/+/39615/17/src/mainboard/google/trogd... PS17, Line 39: i2c_init(QUPV3_0_SE2, I2C_SPEED_FAST); /* EDP Bridge I2C */
Please move this to a separate function because it's no longer "just" loading QUPs now (e.g. […]
Done
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/in... File src/soc/qualcomm/common/include/soc/sn65dsi86bridge.h:
PS17:
This part is not Qualcomm-specific, the driver should go in src/drivers/ti/sn65dsi86. […]
Done
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/in... PS17, Line 2: * This file is part of the coreboot project.
coreboot's license policy just changed, these should all just be one line […]
Done
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/in... PS17, Line 16: __SN65DSI86_DP_H
Header guard should match the name of the file (and ideally the name of the directory too).
Done
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/in... PS17, Line 105: SEL_12MHZ,
Please prefix all constants in this header with something to avoid naming clashes, e.g. […]
Done
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/in... PS17, Line 108: SEL_27MHZ,
enums where the actual numerical value of the constant is important should always explicitly assign […]
Done
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/in... PS17, Line 144: int sn65dsi86_bridge_init(struct msm_fb_panel_data *panel);
Blank line before #endif.
Done
https://review.coreboot.org/c/coreboot/+/39615/16/src/soc/qualcomm/common/sn... File src/soc/qualcomm/common/sn65dsi86bridge.c:
https://review.coreboot.org/c/coreboot/+/39615/16/src/soc/qualcomm/common/sn... PS16, Line 16: #include <device/mmio.h> : #include <console/console.h> : #include <delay.h> : #include <device/device.h> : #include <device/i2c_simple.h> : #include <edid.h> : #include <string.h> : #include <timer.h> : #include <types.h> : #include <soc/addressmap.h> : #include <soc/display/msm_panel.h> : #include <soc/display.h> : #include <soc/sn65dsi86bridge.h>
clean please
Done
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/sn... File src/soc/qualcomm/common/sn65dsi86bridge.c:
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/sn... PS17, Line 90: reg, 0x1)
Shouldn't this be reg++, *data++?
Done
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/sn... PS17, Line 97: mdelay(100);
I assume this is why our boot takes sooooo long to get past display init... […]
I dont find any slave address for the DPCD read/write to implement using direct method.
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/sn... PS17, Line 232:
You need to check that dp_rate_idx didn't run over the end without finding a valid rate here.
Done
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/sn... PS17, Line 267: uint8_t data;
I'm unclear why you have both buf and data here, can't you just use one everywhere?
Done
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/sn... PS17, Line 272: 500000
wait_us(500000, ...) can be written as wait_ms(500, ...).
Done
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/sn... PS17, Line 273: (
Don't really need the outermost parenthesis here.
Done
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/sn... PS17, Line 274: ((buf & BIT(7))))))
I think this needs to be aligned with the line above (not indented further), that's what the bot is […]
Done
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/sn... PS17, Line 293: printk(BIOS_ERR, "Link training failed");
nit: Put "ERROR: " before serious errors to make them more noticeable.
Done
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/sn... PS17, Line 305: i2c_writeb(I2C_BUS, CHIP, SN_HPD_DISABLE_REG, buf);
use i2c_write_field()
Done
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/sn... PS17, Line 314: if (buf & HPD_DISABLE)
This should be […]
Done
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/sn... PS17, Line 332: stopwatch_init_msecs_expire(&hpd, 360);
You can write this whole thing as […]
Done
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/sn... PS17, Line 347: ret = i2c_readb(I2C_BUS, CHIP, SN_ENH_FRAME_REG, &buf);
I don't understand why you're reading here? i2c_write_field() already does the read-modify-write.
Done
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/sn... PS17, Line 364: struct msm_fb_panel_data *panel
This should be a generic driver so do not use Qualcomm-specific data structures. […]
Done
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/sn... PS17, Line 384: SEL_19MHZ
Since other boards might use another reference clock, this should probably be passed in as a paramet […]
Done
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/sn... PS17, Line 387: I2C_BUS, CHIP
Since this should be a generic driver, we can't hardcode these obviously. […]
Done
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). […]
Done
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.
Done
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 actuall […]
Done
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 […]
Done
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.
Done
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, nothi […]
Done
https://review.coreboot.org/c/coreboot/+/39615/16/src/soc/qualcomm/sc7180/di... File src/soc/qualcomm/sc7180/display.c:
https://review.coreboot.org/c/coreboot/+/39615/16/src/soc/qualcomm/sc7180/di... PS16, Line 16: #include <stdlib.h> : #include <console/console.h> : #include <delay.h> : #include <device/device.h> : #include <device/i2c_simple.h> : #include <soc/display/mdssreg.h> : #include <soc/display/mipi_dsi.h> : #include <soc/display/panel.h> : #include <soc/display/msm_panel.h> : #include <soc/display/panel_display.h> : #include <soc/display/target_sc7180.h> : #include <soc/display.h> : #include <soc/sn65dsi86bridge.h>
same […]
Done
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/sc7180/di... File src/soc/qualcomm/sc7180/display.c:
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/sc7180/di... PS17, Line 69: static int sn65dsi86_bridge_read_edid(struct edid *out)
Oh here you have the EDID stuff. […]
Done
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/sc7180/di... PS17, Line 79: 0xA1
This should be written as […]
Done
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/sc7180/di... PS17, Line 80: mdelay(200);
Why the wait? The bridge datasheet doesn't say anything about waiting here (especially not 200ms). […]
this is required for panel coming up. Moved it before edid read call.
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/sc7180/di... PS17, Line 83: ret = i2c_read_bytes(I2C_BUS, EDID_I2C_ADDR, 0x0, edid, EDID_LENGTH);
Oh, and here you're already using the "direct" AUX channel access thing. Very good. […]
Done
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/sc7180/di... PS17, Line 152: pan_type = init_panel_data(&panelstruct,
There should be no concept of a "panel" in the SoC code beyond what struct EDID contains (and maybe […]
Done
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/sc7180/di... PS17, Line 156: if (pan_type == PANEL_TYPE_DSI) {
(This is just an example of stuff that seems unnecessary... […]
Done
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/sc7180/di... PS17, Line 162: panel.fb.width = edid->x_resolution;
You should be passing struct edid (or struct edid_mode) around rather than defining your own structu […]
Done
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 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 p […]
Done
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, P […]
Done
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. […]
Done
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. […]
Done
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. […]
Done
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/sc7180/in... File src/soc/qualcomm/sc7180/include/soc/display/panel_sn65dsix6_auo_bll6xak01_dsi_video.h:
PS17:
As mentioned in the display.c comments, this whole file basically shouldn't exist.
Done
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/sc7180/in... PS17, Line 25: static char slave_panel_node_id[256] = "qcom,dsi_sn65dsix6_auo_b116xak01_video";
None of these things are necessary to put an image on that screen.
Done
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/sc7180/in... PS17, Line 36: 60
You can get this from (struct edid).refresh.
Done
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/sc7180/in... PS17, Line 53: 24
Information about the MIPI connection between panel and bridge can probably just be passed directly […]
Done
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/sc7180/in... PS17, Line 85: 0x0, 0x11, 0x3, 0x5, 0x1E, 0x1E, 0x4, 0x4, 0x2, 0x3, 0x4
Okay, I'm fully lost at this point... I don't know what any of this is. […]
Done
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. […]
Done
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. […]
Done