64 comments:
File src/mainboard/google/trogdor/Kconfig:
Patch Set #25, Line 21: select DRIVERS_TI_SN65DSI86BRIDGE
Your patches have to be in the right order, so if this patch depends on the bridge driver, you have […]
Done
File src/mainboard/google/trogdor/mainboard.c:
#include <device/i2c_simple.h>
#include <soc/display/msm_panel.h>
#include <soc/display.h>
#include <soc/qupv3_i2c.h>
please, clean up
Done
File src/mainboard/google/trogdor/mainboard.c:
Patch Set #17, 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
File src/mainboard/google/trogdor/mainboard.c:
Patch Set #25, Line 60: gpio_configure(GPIO(12), GPIO_FUNC_GPIO, GPIO_NO_PULL,
You should just use gpio_output(GPIO(12), 1) instead of calling both of these manually.
Done
Patch Set #25, Line 68: static void configure_display(int enable)
If we're never calling this with enable == 0, we don't need that parameter and the code depending on […]
Done
Patch Set #25, Line 73: gpio_configure(GPIO(14), GPIO_FUNC_GPIO, GPIO_NO_PULL,
This is only correct for Trogdor rev0, for later boards (e.g. […]
Done
Patch Set #25, Line 80: gpio_set(GPIO(106), 1);
This one also changed, it was 106 for Trogdor rev0, rev1 and Lazor rev0, but will move to GPIO(30) f […]
Done
Patch Set #25, Line 87: uint8_t num_of_lanes, uint8_t bpp
edit: also, let's name the 'bpp' here 'dsi_bpp' or something like that to differentiate it from the […]
Done
Patch Set #25, Line 94: if (ret)
nit: just write […]
Done
Patch Set #25, Line 97: ctrl_mode = read32(&dsi0->ctrl);
Let's not open-code this in the mainboard file... […]
Done
Patch Set #25, Line 101: mdelay(500);
Why do we need this delay? 500ms is very long.
Done
Please make named constants for BRIDGE_BUS and BRIDGE_CHIP at the top of this file.
Done
Patch Set #25, Line 123: edid_set_framebuffer_bits_per_pixel(&ed,
You don't really need to call this since it is already the default. […]
Done
File src/soc/qualcomm/common/include/soc/sn65dsi86bridge.h:
This part is not Qualcomm-specific, the driver should go in src/drivers/ti/sn65dsi86. […]
Done
Patch Set #17, Line 2: * This file is part of the coreboot project.
coreboot's license policy just changed, these should all just be one line […]
Done
Patch Set #17, Line 16: __SN65DSI86_DP_H
Header guard should match the name of the file (and ideally the name of the directory too).
Done
Patch Set #17, Line 105: SEL_12MHZ,
Please prefix all constants in this header with something to avoid naming clashes, e.g. […]
Done
Patch Set #17, Line 108: SEL_27MHZ,
enums where the actual numerical value of the constant is important should always explicitly assign […]
Done
Patch Set #17, Line 144: int sn65dsi86_bridge_init(struct msm_fb_panel_data *panel);
Blank line before #endif.
Done
File src/soc/qualcomm/common/sn65dsi86bridge.c:
#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
File src/soc/qualcomm/common/sn65dsi86bridge.c:
Patch Set #17, Line 90: reg, 0x1)
Shouldn't this be reg++, *data++?
Done
Patch Set #17, Line 97: mdelay(100);
I assume this is why our boot takes sooooo long to get past display init... […]
Done
You need to check that dp_rate_idx didn't run over the end without finding a valid rate here.
Done
Patch Set #17, Line 267: uint8_t data;
I'm unclear why you have both buf and data here, can't you just use one everywhere?
Done
Patch Set #17, Line 272: 500000
wait_us(500000, ...) can be written as wait_ms(500, ...).
Done
Don't really need the outermost parenthesis here.
Done
Patch Set #17, 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
Patch Set #17, Line 293: printk(BIOS_ERR, "Link training failed");
nit: Put "ERROR: " before serious errors to make them more noticeable.
Done
Patch Set #17, Line 305: i2c_writeb(I2C_BUS, CHIP, SN_HPD_DISABLE_REG, buf);
use i2c_write_field()
Done
Patch Set #17, Line 314: if (buf & HPD_DISABLE)
This should be […]
Done
Patch Set #17, Line 332: stopwatch_init_msecs_expire(&hpd, 360);
You can write this whole thing as […]
Done
Patch Set #17, 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
Patch Set #17, Line 364: struct msm_fb_panel_data *panel
This should be a generic driver so do not use Qualcomm-specific data structures. […]
Done
Patch Set #17, Line 384: SEL_19MHZ
Since other boards might use another reference clock, this should probably be passed in as a paramet […]
Done
Patch Set #17, Line 387: I2C_BUS, CHIP
Since this should be a generic driver, we can't hardcode these obviously. […]
Done
File src/soc/qualcomm/sc7180/display.c:
Patch Set #2, 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
Patch Set #2, Line 32: extern int msm_display_init(struct msm_fb_panel_data *pdata);
External function prototypes should go into header files.
Done
Patch Set #2, 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
Patch Set #2, 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
Patch Set #2, Line 55: // if (panelstruct.panelresetseq) //TODO: check if bridge reset seq?
Don't commit commented-out code, ever.
Done
Patch Set #2, 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
File src/soc/qualcomm/sc7180/display.c:
#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
File src/soc/qualcomm/sc7180/display.c:
Patch Set #17, Line 69: static int sn65dsi86_bridge_read_edid(struct edid *out)
Oh here you have the EDID stuff. […]
Done
This should be written as […]
Done
Patch Set #17, Line 80: mdelay(200);
Ack. […]
Yes, panel is not up below 200ms.
Patch Set #17, 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
Patch Set #17, 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
Patch Set #17, Line 156: if (pan_type == PANEL_TYPE_DSI) {
(This is just an example of stuff that seems unnecessary... […]
Done
Patch Set #17, 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
File src/soc/qualcomm/sc7180/display/mdss.c:
Patch Set #2, 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 co […]
Done
Patch Set #2, 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
Patch Set #2, 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
Patch Set #2, Line 678: MDP_CTL_0_BASE + CTL_START
Register accesses are supposed to be done through struct overlays. […]
Done
Patch Set #2, Line 678: writel
The official coreboot register accessors are read32()/write32(). Do not define your own. […]
Done
File src/soc/qualcomm/sc7180/display/mdss.c:
Patch Set #25, Line 16: int mdp_dsi_cmd_off(void);
You should never need to declare prototypes in .c files. […]
Done
Patch Set #25, Line 159: MDP_VBIF_RT_BASE + 0x568
Don't do this, define a struct overlay for these registers like you have for the others.
Done
File src/soc/qualcomm/sc7180/display/oem_panel.c:
Patch Set #2, 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
File src/soc/qualcomm/sc7180/include/soc/display/panel_sn65dsix6_auo_bll6xak01_dsi_video.h:
As mentioned in the display.c comments, this whole file basically shouldn't exist.
Done
Patch Set #17, 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
You can get this from (struct edid).refresh.
Done
Information about the MIPI connection between panel and bridge can probably just be passed directly […]
Done
Patch Set #17, 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
File src/soc/qualcomm/sc7180/include/soc/display/panel_sn65dsix6_auo_bll6xak01_dsi_video.h:
Patch Set #2, Line 231: {0x4, sn65dsix6_auo_b116xak01_dsi_video_on_cmd30, 0x10},
Do not conflate panel and bridge configuration. […]
Done
File src/soc/qualcomm/sc7180/soc.c:
Patch Set #2, Line 38: sc7180_display_init(dev);
This should be kicked off from trogdor/mainboard.c, then you don't need that #if either. […]
Done
To view, visit change 39615. To unsubscribe, or for help writing mail filters, visit settings.