Ravi kumar has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39615 )
Change subject: sc7180: Add display hardware pipe line initialization [patch 3 of 3] ......................................................................
sc7180: Add display hardware pipe line initialization [patch 3 of 3]
Add sc7180 display hardware pipeline programming support and invoke the display initialization from soc_init
Change-Id: I7d5e3f1781c48759553243abeb3d694f76cd008e Signed-off-by: Vinod Polimera vpolimer@codeaurora.org --- M src/soc/qualcomm/sc7180/Kconfig M src/soc/qualcomm/sc7180/Makefile.inc A src/soc/qualcomm/sc7180/display.c A src/soc/qualcomm/sc7180/display/mdss.c A src/soc/qualcomm/sc7180/display/oem_panel.c A src/soc/qualcomm/sc7180/include/soc/display.h A src/soc/qualcomm/sc7180/include/soc/display/panel_sn65dsix6_auo_bll6xak01_dsi_video.h M src/soc/qualcomm/sc7180/soc.c 8 files changed, 1,448 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/39615/1
diff --git a/src/soc/qualcomm/sc7180/Kconfig b/src/soc/qualcomm/sc7180/Kconfig index 5ef5360..b07f518 100644 --- a/src/soc/qualcomm/sc7180/Kconfig +++ b/src/soc/qualcomm/sc7180/Kconfig @@ -14,6 +14,9 @@ select SOC_QUALCOMM_COMMON select HAVE_UART_SPECIAL select BOOTBLOCK_CONSOLE + select MAINBOARD_HAS_NATIVE_VGA_INIT + select MAINBOARD_FORCE_NATIVE_VGA_INIT + select HAVE_LINEAR_FRAMEBUFFER
if SOC_QUALCOMM_SC7180
diff --git a/src/soc/qualcomm/sc7180/Makefile.inc b/src/soc/qualcomm/sc7180/Makefile.inc index 7df4f6e..8bdd329 100644 --- a/src/soc/qualcomm/sc7180/Makefile.inc +++ b/src/soc/qualcomm/sc7180/Makefile.inc @@ -59,6 +59,14 @@ ramstage-y += qupv3_config.c ramstage-y += qcom_qup_se.c ramstage-$(CONFIG_DRIVERS_UART) += qupv3_uart.c +ramstage-$(CONFIG_MAINBOARD_DO_NATIVE_VGA_INIT) += display/oem_panel.c +ramstage-$(CONFIG_MAINBOARD_DO_NATIVE_VGA_INIT) += display/mdss.c +ramstage-$(CONFIG_MAINBOARD_DO_NATIVE_VGA_INIT) += display/target_sc7180.c +ramstage-$(CONFIG_MAINBOARD_DO_NATIVE_VGA_INIT) += display/dsi_panel_display.c +ramstage-$(CONFIG_MAINBOARD_DO_NATIVE_VGA_INIT) += display/dsi_phy_pll.c +ramstage-$(CONFIG_MAINBOARD_DO_NATIVE_VGA_INIT) += display/dsi_phy.c +ramstage-$(CONFIG_MAINBOARD_DO_NATIVE_VGA_INIT) += display/dsi.c +ramstage-$(CONFIG_MAINBOARD_DO_NATIVE_VGA_INIT) += display.c
################################################################################
diff --git a/src/soc/qualcomm/sc7180/display.c b/src/soc/qualcomm/sc7180/display.c new file mode 100644 index 0000000..1759179 --- /dev/null +++ b/src/soc/qualcomm/sc7180/display.c @@ -0,0 +1,275 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2020 Qualcomm Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + + +#include <stdlib.h> +#include <console/console.h> +#include <delay.h> +#include <device/device.h> +#include <soc/mdss_6_2_0.h> +#include <soc/display/panel.h> +#include <soc/display/mipi_dsi.h> +#include <soc/display/panel_display.h> +#include <soc/display/target_sc7180.h> +#include <soc/display.h> + +static struct msm_fb_panel_data panel; +struct panel_struct panelstruct; +static uint8_t display_enable; +static struct mdss_dsi_phy_ctrl dsi_video_mode_phy_db; +extern int msm_display_init(struct msm_fb_panel_data *pdata); +int msm_display_config(void); + +static int update_dsi_display_config(void) +{ + /* Place holder to handle dual dsi cases in future */ + int ret = NO_ERROR; + return ret; +} + +static uint32_t mdss_dsi_panel_clock(uint8_t enable, + struct msm_panel_info *pinfo) +{ + /* MDSS Clocks are set in QCLib. DSI clocks are set after programming + * DSI Phy and Pll. This will be place holder if we want to enable + * MDSS Clocks in coreboot later. + */ + return NO_ERROR; +} + +static uint32_t mdss_dsi_panel_reset(uint8_t enable) +{ + uint32_t ret = NO_ERROR; +// if (panelstruct.panelresetseq) //TODO: check if bridge reset seq? + ret = target_panel_reset(enable, panelstruct.panelresetseq, + &panel.panel_info); + return ret; +} + +static int mdss_dsi_panel_power(uint8_t enable, + struct msm_panel_info *pinfo) +{ + int ret = NO_ERROR; + if (enable) { + ret = mdss_dsi_panel_reset(enable); + if (ret) { + printk(BIOS_ERR, "panel reset failed\n"); + return ret; + } + } else { + ret = mdss_dsi_panel_reset(enable); + if (ret) { + printk(BIOS_ERR, "panel reset disable failed\n"); + return ret; + } + } + + return ret; +} + +static int mdss_dsi_panel_pre_init(void) +{ + int ret = NO_ERROR; + if (panelstruct.paneldata->panel_lp11_init) { + ret = mdss_dsi_panel_reset(1); + if (ret) { + printk(BIOS_ERR, "panel reset failed\n"); + return ret; + } + } + if (panelstruct.paneldata->panel_init_delay) + udelay(panelstruct.paneldata->panel_init_delay); + + printk(BIOS_INFO, "Panel pre init done\n"); + return ret; +} + +static int mdss_dsi_bl_enable(uint8_t enable) +{ + int ret = NO_ERROR; + + ret = target_backlight_ctrl(enable); + if (ret) + printk(BIOS_ERR, "Backlight %s failed\n", enable ? "enable" : + "disable"); + return ret; +} + +int msm_display_config(void) +{ + int ret = NO_ERROR; + struct msm_panel_info *pinfo; + pinfo = &(panel.panel_info); + + mdp_set_revision(panel.mdp_rev); + + switch (pinfo->type) { + case MIPI_VIDEO_PANEL: + printk(BIOS_INFO, "Config MIPI_VIDEO_PANEL.\n"); + ret = mdss_dsi_config(&panel); + if (ret) + goto msm_display_config_out; + + if (pinfo->early_config) + ret = pinfo->early_config((void *)pinfo); + + ret = mdp_dsi_video_config(pinfo, &(panel.fb)); + + if (ret) + goto msm_display_config_out; + + break; + default: + return ERROR; + } + if (pinfo->config) + ret = pinfo->config((void *)pinfo); + +msm_display_config_out: + return ret; +} + +int msm_display_init(struct msm_fb_panel_data *pdata) +{ + int ret = NO_ERROR; + + printk(BIOS_INFO, "%s: Display Initialization start\n", __func__); + if (!pdata) { + ret = ERROR; + goto msm_display_init_out; + } + /* Turn on panel */ + if (pdata->power_func) + ret = pdata->power_func(1, &(pdata->panel_info)); + + if (ret) + goto msm_display_init_out; + + /* Enable clock */ + if (pdata->clk_func) + ret = pdata->clk_func(1, &(pdata->panel_info)); + if (ret) + goto msm_display_init_out; + + if (pdata->update_panel_info) + ret = pdata->update_panel_info(); + + if (ret) + goto msm_display_init_out; + + if (pdata->pll_clk_func) + ret = pdata->pll_clk_func(1, &(pdata->panel_info)); + if (ret) + goto msm_display_init_out; + + /* pinfo prepare */ + if (pdata->panel_info.prepare) { + /* this is for edp which pinfo derived from edid */ + ret = pdata->panel_info.prepare(); + pdata->fb.width = pdata->panel_info.xres; + pdata->fb.height = pdata->panel_info.yres; + pdata->fb.stride = pdata->panel_info.xres; + pdata->fb.bpp = pdata->panel_info.bpp; + } + + if (ret) + goto msm_display_init_out; + + ret = msm_display_config(); + + if (ret) + goto msm_display_init_out; + + ret = mdp_dsi_video_on(&(pdata->panel_info)); + + if (ret) + goto msm_display_init_out; + + if (pdata->post_power_func) + ret = pdata->post_power_func(1); + + if (ret) + goto msm_display_init_out; + + /* Turn on backlight */ + if (pdata->bl_func) + ret = pdata->bl_func(1); + if (ret) + goto msm_display_init_out; + +msm_display_init_out: + return ret; +} + +/* This has to be called from soc init, with right MDP rev */ +int display_init(const char *panel_name, uint32_t rev, struct edid *ed) +{ + int ret = NO_ERROR; + int pan_type; + dsi_video_mode_phy_db.pll_type = DSI_PLL_TYPE_10NM; + printk(BIOS_INFO, " display init!\n"); + pan_type = oem_panel_select(panel_name, &panelstruct, + &(panel.panel_info), + &dsi_video_mode_phy_db); + if (pan_type == PANEL_TYPE_DSI) { + if (update_dsi_display_config()) + goto error_display_init; + target_dsi_phy_config(&dsi_video_mode_phy_db); + if (dsi_panel_init(&(panel.panel_info), &panelstruct)) { + printk(BIOS_ERR, "DSI panel init failed!\n"); + ret = ERROR; + goto error_display_init; + } + panel.panel_info.mipi.mdss_dsi_phy_db = &dsi_video_mode_phy_db; + panel.pll_clk_func = mdss_dsi_panel_clock; + panel.power_func = mdss_dsi_panel_power; + panel.pre_init_func = mdss_dsi_panel_pre_init; + panel.bl_func = mdss_dsi_bl_enable; + + panel.fb.base = NULL; /* Filled in DC */ + panel.fb.width = panel.panel_info.xres; + panel.fb.height = panel.panel_info.yres; + panel.fb.stride = panel.panel_info.xres; + panel.fb.bpp = panel.panel_info.bpp; + panel.fb.format = panel.panel_info.mipi.dst_format; + panel.fb.sbpp = FRAMEBUFFER_SRC_BPP; + if (ed != NULL) { + ed->mode.ha = panel.panel_info.xres; + ed->mode.va = panel.panel_info.yres; + ed->framebuffer_bits_per_pixel = FRAMEBUFFER_SRC_BPP; + } + } else { + printk(BIOS_ERR, "Target panel init not found!\n"); + ret = ERROR; + goto error_display_init; + } + panel.mdp_rev = rev; + ret = msm_display_init(&panel); + +error_display_init: + display_enable = ret ? 0 : 1; + return ret; +} + +void sc7180_display_init(struct device *dev) +{ + /* Restricting display hw initialization to cheza boards only */ + static struct edid ed; + printk(BIOS_INFO, "sc7180 display init!\n"); + display_init(NULL, MDSS_MDP_HW_REV_620, &ed); + edid_set_framebuffer_bits_per_pixel(&ed, ed.framebuffer_bits_per_pixel, + 0); + set_vbe_mode_info_valid(&ed, (uintptr_t)0); +} diff --git a/src/soc/qualcomm/sc7180/display/mdss.c b/src/soc/qualcomm/sc7180/display/mdss.c new file mode 100644 index 0000000..f949729 --- /dev/null +++ b/src/soc/qualcomm/sc7180/display/mdss.c @@ -0,0 +1,679 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2020 Qualcomm Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <arch/mmio.h> +#include <stdlib.h> +#include <console/console.h> +#include <delay.h> +#include <soc/mdss_6_2_0.h> +#include <soc/display/target_sc7180.h> +#include <soc/display/msm_panel.h> +#include <soc/display/mipi_dsi.h> + +#define MDSS_MDP_MAX_PREFILL_FETCH 25 + +uint32_t mdss_mdp_intf_offset(void); + +void mdss_layer_mixer_setup(struct fbcon_config *fb, struct msm_panel_info + *pinfo); + +void mdss_vbif_qos_remapper_setup(struct msm_panel_info *pinfo); + +int mdp_dsi_video_off(struct msm_panel_info *pinfo); + +int mdp_dsi_cmd_off(void); + +static int mdp_rev; + +void mdp_set_revision(int rev) +{ + mdp_rev = rev; +} + +int mdp_get_revision(void) +{ + return mdp_rev; +} + +uint32_t mdss_mdp_intf_offset(void) +{ + return 0; +} + +static uint32_t mdss_mdp_get_ppb_offset(void) +{ + uint32_t mdss_mdp_ppb_off = 0; + uint32_t mdss_mdp_rev = readl(MDP_HW_REV); + + /* return MMSS_MDP_PPB0_CONFIG offset from MDSS base */ + if (mdss_mdp_rev == MDSS_MDP_HW_REV_620) + mdss_mdp_ppb_off = 0x1420; //TODO: check if it is required atall + else + printk(BIOS_ERR, "Invalid PPB0_CONFIG offset\n"); + + return mdss_mdp_ppb_off; +} + +static void mdp_select_pipe_type(struct msm_panel_info *pinfo, + uint32_t *left_pipe, uint32_t *right_pipe) +{ + switch (pinfo->pipe_type) { + + case MDSS_MDP_PIPE_TYPE_DMA: + *left_pipe = MDP_VP_0_DMA_0_BASE; + *right_pipe = MDP_VP_0_DMA_1_BASE; + break; + + case MDSS_MDP_PIPE_TYPE_VIG: + default: + *left_pipe = MDP_VP_0_VIG_0_BASE; + *right_pipe = MDP_VP_0_VIG_1_BASE; + break; + } +} + +static void mdss_mdp_set_flush(struct msm_panel_info *pinfo, + uint32_t *ctl0_reg_val, uint32_t *ctl1_reg_val) +{ + bool dual_pipe_single_ctl = pinfo->lcdc.dual_pipe && + !pinfo->mipi.dual_dsi && + !pinfo->lcdc.split_display; + + *ctl0_reg_val = BIT(17); /*attach ctl path */ + *ctl1_reg_val = BIT(17); + + switch (pinfo->pipe_type) { + + case MDSS_MDP_PIPE_TYPE_DMA: + if (dual_pipe_single_ctl) + *ctl0_reg_val |= BIT(12)| BIT(11) | BIT(6) | BIT(7); + else + *ctl0_reg_val |= BIT(12) | BIT(11) | BIT(6); + + *ctl1_reg_val |= BIT(12)| BIT(7); + + if (pinfo->lcdc.dst_split) + *ctl0_reg_val |= BIT(12); + break; + + case MDSS_MDP_PIPE_TYPE_VIG: + default: + if (dual_pipe_single_ctl) + *ctl0_reg_val |= BIT(0)| BIT(1) |BIT(6) | BIT(7); + else + *ctl0_reg_val |= BIT(0)| BIT(6); + + *ctl1_reg_val |= BIT(7)|BIT(1); + + if (pinfo->lcdc.dst_split) + *ctl0_reg_val |= BIT(1); + + break; + } + + *ctl0_reg_val |= BIT(31); /*Interface flush */ + *ctl1_reg_val |= BIT(31); +} + +static void mdss_source_pipe_config(struct fbcon_config *fb, + struct msm_panel_info *pinfo, + uint32_t pipe_base) +{ + uint32_t img_size, out_size, stride; + uint32_t fb_off = 0; + uint32_t flip_bits = 0; + uint32_t src_xy = 0; + uint32_t dst_xy = 0; + uint32_t height, width; + + height = fb->height - pinfo->border_top - pinfo->border_bottom; + width = fb->width - pinfo->border_left - pinfo->border_right; + + /* write active region size*/ + img_size = (height << 16) | width; + out_size = img_size; + + if (pinfo->lcdc.dual_pipe) { + + if ((pipe_base == MDP_VP_0_DMA_1_BASE) || + (pipe_base == MDP_VP_0_VIG_1_BASE)) { + fb_off = (pinfo->xres / 2); + out_size = (height << 16) + (pinfo->lm_split[1]); + } + else { + out_size = (height << 16) + (pinfo->lm_split[0]); + } + } + + stride = (fb->stride * fb->sbpp/8); + + if (fb_off == 0) { /* left */ + dst_xy = (pinfo->border_top << 16) | pinfo->border_left; + src_xy = dst_xy; + } else { /* right */ + dst_xy = (pinfo->border_top << 16); + src_xy = (pinfo->border_top << 16) | fb_off; + } + + printk(BIOS_INFO, "%s: src=%x fb_off=%x src_xy=%x dst_xy=%x\n", + __func__, out_size, fb_off, src_xy, dst_xy); + + writel(stride, pipe_base + PIPE_SSPP_SRC_YSTRIDE); + + writel(out_size, pipe_base + PIPE_SSPP_SRC_SIZE); + writel(out_size, pipe_base + PIPE_SSPP_SRC_OUT_SIZE); + writel(src_xy, pipe_base + PIPE_SSPP_SRC_XY); + writel(dst_xy, pipe_base + PIPE_SSPP_OUT_XY); + + /* Tight Packing 4bpp Alpha 8-bit A R B G */ + writel(0x000236ff, pipe_base + PIPE_SSPP_SRC_FORMAT); + writel(0x03020001, pipe_base + PIPE_SSPP_SRC_UNPACK_PATTERN); + + /* bit(0) is set if hflip is required. + * bit(1) is set if vflip is required. + */ + if (pinfo->orientation & 0x1) + flip_bits |= MDSS_MDP_OP_MODE_FLIP_LR; + if (pinfo->orientation & 0x2) + flip_bits |= MDSS_MDP_OP_MODE_FLIP_UD; + + flip_bits |= BIT(31); + writel(out_size, pipe_base + PIPE_SW_PIXEL_EXT_C0_REQ); + writel(out_size, pipe_base + PIPE_SW_PIXEL_EXT_C1C2_REQ); + writel(out_size, pipe_base + PIPE_SW_PIXEL_EXT_C3_REQ); + writel(flip_bits, pipe_base + PIPE_SSPP_SRC_OP_MODE); + +} + +static void mdss_vbif_setup(void) +{ + writel(0x33333333, VBIF_VBIF_DDR_AXI_AMEMTYPE_CONF0); + writel(0x00333333, VBIF_VBIF_DDR_AXI_AMEMTYPE_CONF1); +} + +static void mdss_intf_tg_setup(struct msm_panel_info *pinfo, uint32_t intf_base) +{ + uint32_t hsync_period, vsync_period; + uint32_t hsync_start_x, hsync_end_x; + uint32_t display_hctl, hsync_ctl, display_vstart, display_vend; + uint32_t adjust_xres = 0; + uint32_t upper = 0, lower = 0; + + struct lcdc_panel_info *lcdc = NULL; + struct intf_timing_params itp = {0}; + + if (pinfo == NULL) + return; + + lcdc = &(pinfo->lcdc); + if (lcdc == NULL) + return; + + adjust_xres = pinfo->xres; + if (pinfo->lcdc.split_display) { + if (pinfo->lcdc.dst_split) { + adjust_xres /= 2; + } else if (pinfo->lcdc.dual_pipe) { + if (intf_base == + (MDP_INTF_1_BASE + mdss_mdp_intf_offset())) + adjust_xres = pinfo->lm_split[0]; + else + adjust_xres = pinfo->lm_split[1]; + } + + if (intf_base == (MDP_INTF_1_BASE + mdss_mdp_intf_offset())) { + if (pinfo->lcdc.pipe_swap) { + lower |= BIT(4); + upper |= BIT(8); + } else { + lower |= BIT(8); + upper |= BIT(4); + } + writel(lower, MDP_REG_SPLIT_DISPLAY_LOWER_PIPE_CTL); + writel(upper, MDP_REG_SPLIT_DISPLAY_UPPER_PIPE_CTL); + writel(0x1, MDP_REG_SPLIT_DISPLAY_EN); + } + } + + if (pinfo->lcdc.dst_split && + (intf_base == (MDP_INTF_1_BASE + mdss_mdp_intf_offset()))) { + uint32_t ppb_offset = mdss_mdp_get_ppb_offset(); + + writel(BIT(5), REG_MDP(ppb_offset)); /* MMSS_MDP_PPB0_CNTL */ + writel(BIT(16) | (0x3 << 20), REG_MDP(ppb_offset + 0x4)); + } + + itp.xres = adjust_xres; + itp.yres = pinfo->yres; + itp.width = (adjust_xres + pinfo->lcdc.xres_pad); + + if (pinfo->compression_mode == COMPRESSION_DSC) { + itp.xres = pinfo->dsc.pclk_per_line; + itp.width = pinfo->dsc.pclk_per_line; + } + + itp.height = pinfo->yres + pinfo->lcdc.yres_pad; + itp.h_back_porch = pinfo->lcdc.h_back_porch; + itp.h_front_porch = pinfo->lcdc.h_front_porch; + itp.v_back_porch = pinfo->lcdc.v_back_porch; + itp.v_front_porch = pinfo->lcdc.v_front_porch; + itp.hsync_pulse_width = pinfo->lcdc.h_pulse_width; + itp.vsync_pulse_width = pinfo->lcdc.v_pulse_width; + + itp.border_clr = pinfo->lcdc.border_clr; + itp.underflow_clr = pinfo->lcdc.underflow_clr; + itp.hsync_skew = pinfo->lcdc.hsync_skew; + + hsync_period = itp.hsync_pulse_width + itp.h_back_porch + + itp.width + itp.h_front_porch; + + vsync_period = itp.vsync_pulse_width + itp.v_back_porch + + itp.height + itp.v_front_porch; + + hsync_start_x = itp.hsync_pulse_width + + itp.h_back_porch; + hsync_end_x = hsync_period - itp.h_front_porch - 1; + + display_vstart = (itp.vsync_pulse_width + + itp.v_back_porch) * + hsync_period + itp.hsync_skew; + + display_vend = ((vsync_period - itp.v_front_porch) * hsync_period) + + itp.hsync_skew - 1; + + if (intf_base == (MDP_INTF_0_BASE + mdss_mdp_intf_offset())) { /* eDP */ + display_vstart += itp.hsync_pulse_width + itp.h_back_porch; + display_vend -= itp.h_front_porch; + } + + hsync_ctl = (hsync_period << 16) | itp.hsync_pulse_width; + display_hctl = (hsync_end_x << 16) | hsync_start_x; + + writel(hsync_ctl, MDP_HSYNC_CTL + intf_base); + writel(vsync_period*hsync_period, MDP_VSYNC_PERIOD_F0 + + intf_base); + writel(itp.vsync_pulse_width*hsync_period, + MDP_VSYNC_PULSE_WIDTH_F0 + + intf_base); + writel(display_hctl, MDP_DISPLAY_HCTL + intf_base); + writel(display_vstart, MDP_DISPLAY_V_START_F0 + + intf_base); + writel(display_vend, MDP_DISPLAY_V_END_F0 + + intf_base); + writel(itp.underflow_clr, MDP_UNDERFFLOW_COLOR + intf_base); + writel(0x2100, MDP_PANEL_FORMAT + intf_base); +} + +static void mdss_intf_fetch_start_config(struct msm_panel_info *pinfo, + uint32_t intf_base) +{ + uint32_t v_total, h_total, fetch_start, vfp_start; + uint32_t prefetch_avail, prefetch_needed; + uint32_t adjust_xres = 0; + uint32_t fetch_enable = BIT(31); + + struct lcdc_panel_info *lcdc = NULL; + + if (pinfo == NULL) + return; + + lcdc = &(pinfo->lcdc); + if (lcdc == NULL) + return; + + /* + * MDP programmable fetch is for MDP with rev >= 1.05. + * Programmable fetch is not needed if vertical back porch + * plus vertical puls width is >= 25. + */ + if ((lcdc->v_back_porch + lcdc->v_pulse_width) >= MDSS_MDP_MAX_PREFILL_FETCH) + return; + + adjust_xres = pinfo->xres; + if (pinfo->lcdc.split_display) { + if (pinfo->lcdc.dst_split) { + adjust_xres /= 2; + } else if (pinfo->lcdc.dual_pipe) { + if (intf_base == (MDP_INTF_1_BASE + mdss_mdp_intf_offset())) + adjust_xres = pinfo->lm_split[0]; + else + adjust_xres = pinfo->lm_split[1]; + } + } + + if (pinfo->compression_mode == COMPRESSION_DSC) + adjust_xres = pinfo->dsc.pclk_per_line; + + /* + * Fetch should always be outside the active lines. If the fetching + * is programmed within active region, hardware behavior is unknown. + */ + v_total = lcdc->v_pulse_width + lcdc->v_back_porch + pinfo->yres + + lcdc->v_front_porch; + h_total = lcdc->h_pulse_width + lcdc->h_back_porch + adjust_xres + + lcdc->h_front_porch; + vfp_start = lcdc->v_pulse_width + lcdc->v_back_porch + pinfo->yres; + + prefetch_avail = v_total - vfp_start; + prefetch_needed = MDSS_MDP_MAX_PREFILL_FETCH - lcdc->v_back_porch - + lcdc->v_pulse_width; + + /* + * In some cases, vertical front porch is too high. In such cases limit + * the mdp fetch lines as the last (25 - vbp - vpw) lines of + * vertical front porch. + */ + if (prefetch_avail > prefetch_needed) + prefetch_avail = prefetch_needed; + + fetch_start = (v_total - prefetch_avail) * h_total+ h_total + 1; + + if (pinfo->dfps.panel_dfps.enabled) + fetch_enable |= BIT(23); //TODO: this shoudld not be enabled ? + + writel(fetch_start, MDP_PROG_FETCH_START + intf_base); + + writel(fetch_enable, MDP_INTF_CONFIG + intf_base); + +} + +void mdss_layer_mixer_setup(struct fbcon_config *fb, struct msm_panel_info + *pinfo) +{ + uint32_t mdp_rgb_size, height, width; + uint32_t left_staging_level, right_staging_level; + + height = fb->height; + width = fb->width; + + if (pinfo->lcdc.dual_pipe && !pinfo->lcdc.dst_split) + width = pinfo->lm_split[0]; + + /* write active region size*/ + mdp_rgb_size = (height << 16) | width; + + writel(mdp_rgb_size, MDP_VP_0_MIXER_0_BASE + LAYER_0_OUT_SIZE); + writel(0x00, MDP_VP_0_MIXER_0_BASE + LAYER_0_OP_MODE); + + for (int i = 0; i < 6; i++) { + writel(0x100, MDP_VP_0_MIXER_0_BASE + LAYER_0_BLEND_OP(i)); + writel(0x00ff0000, MDP_VP_0_MIXER_0_BASE + LAYER_0_BLEND0_CONST_ALPHA(i)); + } + + left_staging_level =BIT(24); //attach border + right_staging_level=BIT(24); + + switch (pinfo->pipe_type) { + case MDSS_MDP_PIPE_TYPE_DMA: + left_staging_level |= BIT(18); + right_staging_level |= BIT(21); + break; + case MDSS_MDP_PIPE_TYPE_VIG: + default: + left_staging_level |= BIT(1); + right_staging_level |= BIT(3); + break; + } + + /* + * When ping-pong split is enabled and two pipes are used, + * both the pipes need to be staged on the same layer mixer. + */ + if (pinfo->lcdc.dual_pipe && pinfo->lcdc.dst_split) + left_staging_level |= right_staging_level; + + /* Base layer for layer mixer 0 */ + writel(left_staging_level, MDP_CTL_0_BASE + CTL_LAYER_0); + + if (pinfo->lcdc.dual_pipe && !pinfo->lcdc.dst_split) { + /* write active region size*/ + mdp_rgb_size = (height << 16) | pinfo->lm_split[1]; + + writel(mdp_rgb_size, MDP_VP_0_MIXER_1_BASE + LAYER_0_OUT_SIZE); + writel(0x00, MDP_VP_0_MIXER_1_BASE + LAYER_0_OP_MODE); + + for (int i = 0; i < 6; i++) { + writel(0x100, MDP_VP_0_MIXER_1_BASE + LAYER_0_BLEND_OP(i)); + writel(0x00ff0000, MDP_VP_0_MIXER_1_BASE + LAYER_0_BLEND0_CONST_ALPHA(i)); + } + + /* Base layer for layer mixer 1 */ + if (pinfo->lcdc.split_display) + writel(right_staging_level, MDP_CTL_1_BASE + CTL_LAYER_1); + else + writel(right_staging_level, MDP_CTL_0_BASE + CTL_LAYER_1); + } +} + +void mdss_vbif_qos_remapper_setup(struct msm_panel_info *pinfo) +{ + writel(0x00000003, MDSS_VBIF_RT_BASE + 0x550); + writel(0x11111113, MDSS_VBIF_RT_BASE + 0x558); + writel(0x22222224, MDSS_VBIF_RT_BASE + 0x560); + writel(0x33333334, MDSS_VBIF_RT_BASE + 0x568); + writel(0x44444445, MDSS_VBIF_RT_BASE + 0x570); + writel(0x77777776, MDSS_VBIF_RT_BASE + 0x588); + writel(0x00000003, MDSS_VBIF_RT_BASE + 0x590); + writel(0x11111113, MDSS_VBIF_RT_BASE + 0x598); + writel(0x22222224, MDSS_VBIF_RT_BASE + 0x5a0); + writel(0x33333334, MDSS_VBIF_RT_BASE + 0x5a8); + writel(0x44444445, MDSS_VBIF_RT_BASE + 0x5b0); + writel(0x77777776, MDSS_VBIF_RT_BASE + 0x5c8); + +} + +static uint32_t mdss_mdp_ctl_out_sel(struct msm_panel_info *pinfo, + int is_main_ctl) +{ + uint32_t mctl_intf_sel; + uint32_t sctl_intf_sel; + + if ((pinfo->dest == DISPLAY_2) || + ((pinfo->dest == DISPLAY_1) && (pinfo->lcdc.pipe_swap))) { + mctl_intf_sel = BIT(4) | BIT(5); /* Interface 2 */ + sctl_intf_sel = BIT(5); /* Interface 1 */ + } else { + mctl_intf_sel = BIT(5); /* Interface 1 */ + sctl_intf_sel = BIT(4) | BIT(5); /* Interface 2 */ + } + printk(BIOS_INFO, "%s: main ctl dest=%s sec ctl dest=%s\n", __func__, + (mctl_intf_sel & BIT(4)) ? "Intf2" : "Intf1", + (sctl_intf_sel & BIT(4)) ? "Intf2" : "Intf1"); + return is_main_ctl ? mctl_intf_sel : sctl_intf_sel; +} + +static void mdp_set_intf_base(struct msm_panel_info *pinfo, + uint32_t *intf_base, uint32_t *sintf_base) +{ + if (pinfo->dest == DISPLAY_2) { + *intf_base = MDP_INTF_2_BASE; + *sintf_base = MDP_INTF_1_BASE; + } else { + *intf_base = MDP_INTF_1_BASE; + *sintf_base = MDP_INTF_2_BASE; + } +} + +int mdp_dsi_video_config(struct msm_panel_info *pinfo, + struct fbcon_config *fb) +{ + uint32_t intf_sel, sintf_sel; + uint32_t intf_base, sintf_base; + uint32_t left_pipe, right_pipe; + uint32_t reg; + + mdp_set_intf_base(pinfo, &intf_base, &sintf_base); + + mdss_intf_tg_setup(pinfo, intf_base); + mdss_intf_fetch_start_config(pinfo, intf_base); + + if (pinfo->mipi.dual_dsi) { + mdss_intf_tg_setup(pinfo, sintf_base); + mdss_intf_fetch_start_config(pinfo, sintf_base); + } + + mdp_select_pipe_type(pinfo, &left_pipe, &right_pipe); + mdss_vbif_setup(); + mdss_vbif_qos_remapper_setup(pinfo); + + mdss_source_pipe_config(fb, pinfo, left_pipe); + + if (pinfo->lcdc.dual_pipe) + mdss_source_pipe_config(fb, pinfo, right_pipe); + + mdss_layer_mixer_setup(fb, pinfo); + reg = mdss_mdp_ctl_out_sel(pinfo, 1); /*Selecting interface 1 */ + + /* enable 3D mux for dual_pipe but single interface config */ + if (pinfo->lcdc.dual_pipe && !pinfo->mipi.dual_dsi && + !pinfo->lcdc.split_display) { + + if (pinfo->num_dsc_enc != 2) + reg |= BIT(19) | BIT(20); + } + + writel(0x0F0000, MDP_INTF_1_BASE + INTF_MUX); + writel(0x0, MDP_CTL_0_BASE + CTL_TOP); + writel(BIT(1), MDP_CTL_0_BASE + CTL_INTF_ACTIVE); /*Selecting interface 1 */ + + if ((pinfo->compression_mode == COMPRESSION_DSC) && + pinfo->dsc.mdp_dsc_config) { + struct dsc_desc *dsc = &pinfo->dsc; + + if (pinfo->lcdc.dual_pipe && !pinfo->mipi.dual_dsi && + !pinfo->lcdc.split_display && (pinfo->num_dsc_enc == 2)) { + dsc->mdp_dsc_config(pinfo, MDP_PP_0_BASE, + MDP_DSC_0_BASE, true, true); + + dsc->mdp_dsc_config(pinfo, MDP_PP_1_BASE, + MDP_DSC_1_BASE, true, true); + + } else if (pinfo->lcdc.dual_pipe && pinfo->mipi.dual_dsi && + pinfo->lcdc.split_display && (pinfo->num_dsc_enc == 1)) { + dsc->mdp_dsc_config(pinfo, MDP_PP_0_BASE, + MDP_DSC_0_BASE, false, false); + dsc->mdp_dsc_config(pinfo, MDP_PP_1_BASE, + MDP_DSC_1_BASE, false, false); + + } else { + dsc->mdp_dsc_config(pinfo, MDP_PP_0_BASE, + MDP_DSC_0_BASE, false, false); + } + } + + /* + * if dst_split is enabled, intf 1 & 2 needs to be enabled but + * CTL_1 path should not be set since CTL_0 itself is going + * to split after DSPP block and drive both intf. + */ + if (pinfo->mipi.dual_dsi) { + if (!pinfo->lcdc.dst_split) { + reg = 0x1f00 | mdss_mdp_ctl_out_sel(pinfo, 0); + writel(reg, MDP_CTL_1_BASE + CTL_TOP); + } + + intf_sel |= sintf_sel; /* INTF 2 enable */ + } + + return 0; +} + +int mdp_dsi_cmd_config(struct msm_panel_info *pinfo, + struct fbcon_config *fb) +{ + int ret = NO_ERROR; + return ret; +} + +int mdp_dsi_video_on(struct msm_panel_info *pinfo) +{ + uint32_t ctl0_reg_val, ctl1_reg_val; + + writel(0x2, MDP_CTL_0_BASE + CTL_INTF_FLUSH); + mdss_mdp_set_flush(pinfo, &ctl0_reg_val, &ctl1_reg_val); + writel(ctl0_reg_val, MDP_CTL_0_BASE + CTL_FLUSH); + + if (pinfo->lcdc.dual_pipe && !pinfo->lcdc.dst_split){ + writel(0x4, MDP_CTL_1_BASE + CTL_INTF_FLUSH); + writel(ctl1_reg_val, MDP_CTL_1_BASE + CTL_FLUSH); + } + + return NO_ERROR; +} + +static void mdp_set_cmd_autorefresh_mode(struct msm_panel_info *pinfo) +{ + uint32_t total_lines = 0, vclks_line = 0, cfg = 0; + + if (!pinfo || (pinfo->type != MIPI_CMD_PANEL) || !pinfo->autorefresh_enable) + return; + + total_lines = pinfo->lcdc.v_front_porch + + pinfo->lcdc.v_back_porch + + pinfo->lcdc.v_pulse_width + + pinfo->border_top + pinfo->border_bottom + + pinfo->yres; + + total_lines *= pinfo->mipi.frame_rate; + + vclks_line = (total_lines) ? 19200000 / total_lines : 0; + vclks_line = vclks_line * pinfo->mipi.frame_rate * 100 / 6000; + + cfg = BIT(19) | vclks_line; + + /* Configure tearcheck VSYNC param */ + writel(cfg, MDP_REG_PP_0_SYNC_CONFIG_VSYNC); + + if (pinfo->lcdc.dst_split) + writel(cfg, MDP_REG_PP_SLAVE_SYNC_CONFIG_VSYNC); + + if (pinfo->lcdc.dual_pipe) + writel(cfg, MDP_REG_PP_1_SYNC_CONFIG_VSYNC); + dsb(); + + /* Enable autorefresh mode */ + writel((BIT(31) | pinfo->autorefresh_framenum), + MDP_REG_PP_0_AUTOREFRESH_CONFIG); + + if (pinfo->lcdc.dst_split) + writel((BIT(31) | pinfo->autorefresh_framenum), + MDP_REG_PP_SLAVE_AUTOREFRESH_CONFIG); + + if (pinfo->lcdc.dual_pipe) + writel((BIT(31) | pinfo->autorefresh_framenum), + MDP_REG_PP_1_AUTOREFRESH_CONFIG); + + dsb(); +} + +int mdp_dma_on(struct msm_panel_info *pinfo) +{ + uint32_t ctl0_reg_val, ctl1_reg_val; + + mdss_mdp_set_flush(pinfo, &ctl0_reg_val, &ctl1_reg_val); + + writel(ctl0_reg_val, MDP_CTL_0_BASE + CTL_FLUSH); + + if (pinfo->lcdc.dual_pipe && !pinfo->lcdc.dst_split) + writel(ctl1_reg_val, MDP_CTL_1_BASE + CTL_FLUSH); + + if (pinfo->autorefresh_enable) + mdp_set_cmd_autorefresh_mode(pinfo); + + writel(0x01, MDP_CTL_0_BASE + CTL_START); + + return NO_ERROR; +} + diff --git a/src/soc/qualcomm/sc7180/display/oem_panel.c b/src/soc/qualcomm/sc7180/display/oem_panel.c new file mode 100644 index 0000000..da90398 --- /dev/null +++ b/src/soc/qualcomm/sc7180/display/oem_panel.c @@ -0,0 +1,172 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2020 Qualcomm Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <string.h> +#include <arch/mmio.h> +#include <lib.h> +#include <stdlib.h> +#include <console/console.h> +#include <delay.h> +#include <soc/display/msm_panel.h> +#include <soc/display/mipi_dsi.h> +#include <soc/display/target_sc7180.h> +#include <soc/display/panel_display.h> +#include <soc/mdss_6_2_0.h> +#include <soc/display/panel_sn65dsix6_auo_bll6xak01_dsi_video.h> + +enum { + HW_PLATFORM_MTP, + HW_PLATFORM_PROTO, +}; + +enum { + SN65DSIX6_AUO_B116XAK01_DSI_VIDEO_PANEL, + UNKNOWN_PANEL +}; + +/* + * The list of panels that are supported on this target. + */ +static struct panel_list supp_panels[] = { + {"sn65dsix6_auo_b116xak01_dsi_video", + SN65DSIX6_AUO_B116XAK01_DSI_VIDEO_PANEL}, +}; + +static uint32_t panel_id; +#define SN65DSIX6_AUO_B116XAK01_DSI_VIDEO_PANEL_DELAY 40 + +int oem_panel_bridge_chip_init(struct msm_panel_info *pinfo); + +int oem_panel_rotation(void) +{ + return NO_ERROR; +} + +int oem_panel_on(void) +{ + if (panel_id == SN65DSIX6_AUO_B116XAK01_DSI_VIDEO_PANEL) { + /* needs extra delay to avoid unexpected artifacts */ + mdelay(SN65DSIX6_AUO_B116XAK01_DSI_VIDEO_PANEL_DELAY); + } + + return NO_ERROR; +} + +int oem_panel_off(void) +{ + return NO_ERROR; +} + +static int init_panel_data(struct panel_struct *panelstruct, + struct msm_panel_info *pinfo, + struct mdss_dsi_phy_ctrl *phy_db) +{ + int pan_type = PANEL_TYPE_DSI; + + switch (panel_id) { + case SN65DSIX6_AUO_B116XAK01_DSI_VIDEO_PANEL: + pan_type = PANEL_TYPE_DSI; + panelstruct->paneldata + = &sn65dsix6_auo_b116xak01_dsi_video_panel_data; + panelstruct->paneldata->panel_with_enable_gpio = 0; + panelstruct->panelres + = &sn65dsix6_auo_b116xak01_dsi_video_panel_res; + panelstruct->color + = &sn65dsix6_auo_b116xak01_dsi_video_color; + panelstruct->videopanel + = &sn65dsix6_auo_b116xak01_dsi_video_panel; + panelstruct->commandpanel + = &sn65dsix6_auo_b116xak01_dsi_video_command_panel; + panelstruct->state + = &sn65dsix6_auo_b116xak01_dsi_video_state; + panelstruct->laneconfig + = &sn65dsix6_auo_b116xak01_dsi_video_lane_config; + panelstruct->paneltiminginfo + = &sn65dsix6_auo_b116xak01_dsi_video_timing_info; + panelstruct->panelresetseq + = &sn65dsix6_auo_b116xak01_dsi_video_panel_reset_seq; + pinfo->mipi.panel_on_cmds + = sn65dsix6_auo_b116xak01_dsi_video_on_command; + pinfo->mipi.num_of_panel_on_cmds + = SN65DSIX6_AUO_B116XAK01_DSI_VIDEO_ON_COMMAND; + pinfo->mipi.panel_off_cmds + = sn65dsix6_auo_b116xak01_dsi_video_off_command; + pinfo->mipi.num_of_panel_off_cmds + = SN65DSIX6_AUO_B116XAK01_DSI_VIDEO_OFF_COMMAND; + memcpy(phy_db->timing, + sn65dsix6_auo_b116xak01_dsi_video_timings, + MAX_TIMING_CONFIG * sizeof(uint32_t)); + break; + default: + case UNKNOWN_PANEL: + pan_type = PANEL_TYPE_UNKNOWN; + break; + } + + return pan_type; +} + +int oem_panel_bridge_chip_init(struct msm_panel_info *pinfo) +{ + return target_set_switch_gpio(1); +} + +int oem_panel_select(const char *panel_name, struct panel_struct *panelstruct, + struct msm_panel_info *pinfo, + struct mdss_dsi_phy_ctrl *phy_db) +{ + uint32_t hw_id = HW_PLATFORM_PROTO; + int32_t panel_override_id; + uint32_t ret = 0; + + phy_db->pll_type = DSI_PLL_TYPE_10NM; + + if (panel_name) { + panel_override_id = panel_name_to_id(supp_panels, + ARRAY_SIZE(supp_panels), panel_name); + + if (panel_override_id < 0) { + printk(BIOS_ERR, "Not able to search the panel:%s\n", + panel_name); + } else if (panel_override_id < UNKNOWN_PANEL) { + panel_id = panel_override_id; + printk(BIOS_INFO, "OEM panel override:%s\n", + panel_name); + goto panel_init; + } + } + + switch (hw_id) { + case HW_PLATFORM_PROTO: + panel_id = SN65DSIX6_AUO_B116XAK01_DSI_VIDEO_PANEL; + pinfo->has_bridge_chip = true; + break; + case HW_PLATFORM_MTP: + default: + printk(BIOS_ERR, "Display not enabled for %d HW type\n" + , hw_id); + return PANEL_TYPE_UNKNOWN; + } + +panel_init: + if (pinfo->has_bridge_chip) { + ret = oem_panel_bridge_chip_init(pinfo); + if (ret) { + printk(BIOS_ERR, "Error initializing bridge chip\n"); + return ret; + } + } + return init_panel_data(panelstruct, pinfo, phy_db); +} diff --git a/src/soc/qualcomm/sc7180/include/soc/display.h b/src/soc/qualcomm/sc7180/include/soc/display.h new file mode 100644 index 0000000..20cd748 --- /dev/null +++ b/src/soc/qualcomm/sc7180/include/soc/display.h @@ -0,0 +1,25 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (c) 2018 Qualcomm Technologies + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef __SOC_QCOM_SC7180_DISPLAY_H__ +#define __SOC_QCOM_SC7180_DISPLAY_H__ + +#include <edid.h> + +void sc7180_display_init(struct device *dev); +int display_init(const char *panel_name, uint32_t rev, struct edid *ed); + +#endif + diff --git a/src/soc/qualcomm/sc7180/include/soc/display/panel_sn65dsix6_auo_bll6xak01_dsi_video.h b/src/soc/qualcomm/sc7180/include/soc/display/panel_sn65dsix6_auo_bll6xak01_dsi_video.h new file mode 100644 index 0000000..a783e03 --- /dev/null +++ b/src/soc/qualcomm/sc7180/include/soc/display/panel_sn65dsix6_auo_bll6xak01_dsi_video.h @@ -0,0 +1,281 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (c) 2020 Qualcomm Technologies + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef _PANEL_SN65DSIX6_AUO_BLL6XAK01_DSI_VIDEO_H_ +#define _PANEL_SN65DSIX6_AUO_BLL6XAK01_DSI_VIDEO_H_ + +#include "panel.h" + +static char panel_node_id[256] = "qcom,dsi_sn65dsix6_auo_b116xak01_video"; +static char panel_controller[256] = "dsi:0:"; /* panel_controller */ +static char panel_compatible[256] = "qcom,mdss-dsi-panel"; +static char panel_destination[256] = "DISPLAY_1"; +static char slave_panel_node_id[256] = "qcom,dsi_sn65dsix6_auo_b116xak01_video"; + +static struct panel_config sn65dsix6_auo_b116xak01_dsi_video_panel_data = { + panel_node_id, + panel_controller, + panel_compatible, + 10, /* panel_interface */ + 0, /* panel_type */ + panel_destination, + 0, /* panel_orientation */ + 0, /* panel_clockrate */ + 60, /* panel_framerate */ + 0, /* panel_channelid */ + 0, /* dsi_virtualchannel_id */ + 0, /* panel_broadcast_mode */ + 0, /* panel_lp11_init */ + 0, /* panel_init_delay */ + 0, /* dsi_stream */ + 0, /* interleave_mode */ + 0, /* panel_bitclock_freq */ + 0, /* panel_operating_mode */ + 0, /* panel_with_enable_gpio */ + 0, /* mode_gpio_state */ + slave_panel_node_id, +}; + +static struct panel_resolution sn65dsix6_auo_b116xak01_dsi_video_panel_res = { + 1366, /* panel_width .*/ + 768, /* panel_height .*/ + 48, /* hfront_porch .*/ + 10, /* hback_porch .*/ + 32, /* hpulse_width .*/ + 0, /* hsync_skew .*/ + 4, /* vfront_porch .*/ + 15, /* vback_porch .*/ + 6, /* vpulse_width .*/ + 0, /* hleft_border .*/ + 0, /* hright_border .*/ + 0, /* vtop_border .*/ + 0, /* vbottom_border .*/ + 0, /* hactive_res */ + 0, /* uint16_t vactive_res */ + 0, /* invert_data_polarity */ + 0, /* invert_vsync_polarity */ + 0, /* invert_hsync_polarity */ +}; + +static struct color_info sn65dsix6_auo_b116xak01_dsi_video_color = { + 24, /* color_format */ + 0, /* color_order */ + 0, /* underflow_color */ + 0, /* border_color */ + 0, /* pixel_packing */ + 0, /* pixel_alignment */ +}; + +static char sn65dsix6_auo_b116xak01_dsi_video_on_cmd1[] = { + 0x5c, 0x01, 0x23, 0x80 +}; + +static char sn65dsix6_auo_b116xak01_dsi_video_on_cmd2[] = { + 0x0a, 0x02, 0x23, 0x80 +}; + +static char sn65dsix6_auo_b116xak01_dsi_video_on_cmd3[] = { + 0x10, 0x20, 0x23, 0x80 +}; + +static char sn65dsix6_auo_b116xak01_dsi_video_on_cmd4[] = { + 0x12, 0x29, 0x23, 0x80 +}; + +static char sn65dsix6_auo_b116xak01_dsi_video_on_cmd5[] = { + 0x5a, 0x05, 0x23, 0x80 +}; + +static char sn65dsix6_auo_b116xak01_dsi_video_on_cmd6[] = { + 0x93, 0x10, 0x23, 0x80 +}; + +static char sn65dsix6_auo_b116xak01_dsi_video_on_cmd7[] = { + 0x94, 0x81, 0x23, 0x80 +}; + +static char sn65dsix6_auo_b116xak01_dsi_video_on_cmd8[] = { + 0x0d, 0x01, 0x23, 0x80 +}; + +static char sn65dsix6_auo_b116xak01_dsi_video_on_cmd9[] = { + 0x64, 0x01, 0x23, 0x80 +}; + +static char sn65dsix6_auo_b116xak01_dsi_video_on_cmd10[] = { + 0x74, 0x00, 0x23, 0x80 +}; + +static char sn65dsix6_auo_b116xak01_dsi_video_on_cmd11[] = { + 0x75, 0x01, 0x23, 0x80 +}; + +static char sn65dsix6_auo_b116xak01_dsi_video_on_cmd12[] = { + 0x76, 0x0a, 0x23, 0x80 +}; + +static char sn65dsix6_auo_b116xak01_dsi_video_on_cmd13[] = { + 0x77, 0x01, 0x23, 0x80 +}; + +static char sn65dsix6_auo_b116xak01_dsi_video_on_cmd14[] = { + 0x78, 0x81, 0x23, 0x80 +}; + +static char sn65dsix6_auo_b116xak01_dsi_video_on_cmd15[] = { + 0x96, 0x0a, 0x23, 0x80 +}; + +static char sn65dsix6_auo_b116xak01_dsi_video_on_cmd16[] = { + 0x20, 0x56, 0x23, 0x80 +}; + +static char sn65dsix6_auo_b116xak01_dsi_video_on_cmd17[] = { + 0x21, 0x05, 0x23, 0x80 +}; + +static char sn65dsix6_auo_b116xak01_dsi_video_on_cmd18[] = { + 0x24, 0x00, 0x23, 0x80 +}; + +static char sn65dsix6_auo_b116xak01_dsi_video_on_cmd19[] = { + 0x25, 0x03, 0x23, 0x80 +}; + +static char sn65dsix6_auo_b116xak01_dsi_video_on_cmd20[] = { + 0x2c, 0x20, 0x23, 0x80 +}; + +static char sn65dsix6_auo_b116xak01_dsi_video_on_cmd21[] = { + 0x2d, 0x00, 0x23, 0x80 +}; + +static char sn65dsix6_auo_b116xak01_dsi_video_on_cmd22[] = { + 0x30, 0x06, 0x23, 0x80 +}; + +static char sn65dsix6_auo_b116xak01_dsi_video_on_cmd23[] = { + 0x31, 0x00, 0x23, 0x80 +}; + +static char sn65dsix6_auo_b116xak01_dsi_video_on_cmd24[] = { + 0x34, 0x0a, 0x23, 0x80 +}; + +static char sn65dsix6_auo_b116xak01_dsi_video_on_cmd25[] = { + 0x36, 0x0f, 0x23, 0x80 +}; + +static char sn65dsix6_auo_b116xak01_dsi_video_on_cmd26[] = { + 0x38, 0x30, 0x23, 0x80 +}; + +static char sn65dsix6_auo_b116xak01_dsi_video_on_cmd27[] = { + 0x3a, 0x04, 0x23, 0x80 +}; + +static char sn65dsix6_auo_b116xak01_dsi_video_on_cmd28[] = { + 0x5b, 0x01, 0x23, 0x80 +}; + +static char sn65dsix6_auo_b116xak01_dsi_video_on_cmd29[] = { + 0x3c, 0x00, 0x23, 0x80 +}; + +static char sn65dsix6_auo_b116xak01_dsi_video_on_cmd30[] = { + 0x5a, 0x0d, 0x23, 0x80 +}; + +static struct mipi_dsi_cmd sn65dsix6_auo_b116xak01_dsi_video_on_command[] = { + {0x4, sn65dsix6_auo_b116xak01_dsi_video_on_cmd1, 0x10}, + {0x4, sn65dsix6_auo_b116xak01_dsi_video_on_cmd2, 0x10}, + {0x4, sn65dsix6_auo_b116xak01_dsi_video_on_cmd3, 0x10}, + {0x4, sn65dsix6_auo_b116xak01_dsi_video_on_cmd4, 0x10}, + {0x4, sn65dsix6_auo_b116xak01_dsi_video_on_cmd5, 0x10}, + {0x4, sn65dsix6_auo_b116xak01_dsi_video_on_cmd6, 0xff}, + {0x4, sn65dsix6_auo_b116xak01_dsi_video_on_cmd7, 0x10}, + {0x4, sn65dsix6_auo_b116xak01_dsi_video_on_cmd8, 0xff}, + {0x4, sn65dsix6_auo_b116xak01_dsi_video_on_cmd9, 0x10}, + {0x4, sn65dsix6_auo_b116xak01_dsi_video_on_cmd10, 0x10}, + {0x4, sn65dsix6_auo_b116xak01_dsi_video_on_cmd11, 0x10}, + {0x4, sn65dsix6_auo_b116xak01_dsi_video_on_cmd12, 0x10}, + {0x4, sn65dsix6_auo_b116xak01_dsi_video_on_cmd13, 0xff}, + {0x4, sn65dsix6_auo_b116xak01_dsi_video_on_cmd14, 0x10}, + {0x4, sn65dsix6_auo_b116xak01_dsi_video_on_cmd15, 0xff}, + {0x4, sn65dsix6_auo_b116xak01_dsi_video_on_cmd16, 0x10}, + {0x4, sn65dsix6_auo_b116xak01_dsi_video_on_cmd17, 0x10}, + {0x4, sn65dsix6_auo_b116xak01_dsi_video_on_cmd18, 0x10}, + {0x4, sn65dsix6_auo_b116xak01_dsi_video_on_cmd19, 0x10}, + {0x4, sn65dsix6_auo_b116xak01_dsi_video_on_cmd20, 0x10}, + {0x4, sn65dsix6_auo_b116xak01_dsi_video_on_cmd21, 0x10}, + {0x4, sn65dsix6_auo_b116xak01_dsi_video_on_cmd22, 0x10}, + {0x4, sn65dsix6_auo_b116xak01_dsi_video_on_cmd23, 0x10}, + {0x4, sn65dsix6_auo_b116xak01_dsi_video_on_cmd24, 0x10}, + {0x4, sn65dsix6_auo_b116xak01_dsi_video_on_cmd25, 0x10}, + {0x4, sn65dsix6_auo_b116xak01_dsi_video_on_cmd26, 0x10}, + {0x4, sn65dsix6_auo_b116xak01_dsi_video_on_cmd27, 0x10}, + {0x4, sn65dsix6_auo_b116xak01_dsi_video_on_cmd28, 0x10}, + {0x4, sn65dsix6_auo_b116xak01_dsi_video_on_cmd29, 0x10}, + {0x4, sn65dsix6_auo_b116xak01_dsi_video_on_cmd30, 0x10}, +}; + +#define SN65DSIX6_AUO_B116XAK01_DSI_VIDEO_ON_COMMAND 30 + +static struct mipi_dsi_cmd sn65dsix6_auo_b116xak01_dsi_video_off_command[] = { +}; + +#define SN65DSIX6_AUO_B116XAK01_DSI_VIDEO_OFF_COMMAND 0 + +static struct command_state sn65dsix6_auo_b116xak01_dsi_video_state = { + 0, 1 +}; + +static struct commandpanel_info sn65dsix6_auo_b116xak01_dsi_video_command_panel = { + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 +}; + +static struct videopanel_info sn65dsix6_auo_b116xak01_dsi_video_panel = { + 0, /* hsync_pulse */ + 0, /* hfp_power_mode */ + 0, /* hbp_power_mode */ + 0, /* hsa_power_mode */ + 1, /* bllp_eof_power_mode */ + 1, /* bllp_power_mode */ + 0, /* traffic_mode */ + 0, /* dma_delayafter_vsync */ + 0x9, /* bllp_eof_power */ +}; + +static struct lane_configuration sn65dsix6_auo_b116xak01_dsi_video_lane_config = { +/* For Proto0, due to the incorrect schematic we have to set + force_clk_lane_hs because we are supporting only 384Mhz continuous clock +*/ + 4, 0, 1, 1, 1, 1, 1 +}; + +static const uint32_t sn65dsix6_auo_b116xak01_dsi_video_timings[] = { + 0x0, 0x11, 0x3, 0x5, 0x1E, 0x1E, 0x4, 0x4, 0x2, 0x3, 0x4 +}; + +static struct panel_timing sn65dsix6_auo_b116xak01_dsi_video_timing_info = { + 0x0, 0x04, 0x0c, 0x2d +}; + +static struct panel_reset_sequence + sn65dsix6_auo_b116xak01_dsi_video_panel_reset_seq = { + {1, 0, 1, }, {20, 20, 50, }, 2 +}; + +#endif diff --git a/src/soc/qualcomm/sc7180/soc.c b/src/soc/qualcomm/sc7180/soc.c index fbcfc6e..c0b11ae 100644 --- a/src/soc/qualcomm/sc7180/soc.c +++ b/src/soc/qualcomm/sc7180/soc.c @@ -1,7 +1,7 @@ /* * This file is part of the coreboot project. * - * Copyright (C) 2018-2019, The Linux Foundation. All rights reserved. + * Copyright (C) 2018-2020, The Linux Foundation. All rights reserved. * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License version 2 and @@ -19,6 +19,7 @@ #include <soc/mmu_common.h> #include <soc/symbols.h> #include <soc/aop.h> +#include <soc/display.h>
static void soc_read_resources(struct device *dev) { @@ -33,6 +34,9 @@ static void soc_init(struct device *dev) { aop_fw_load_reset(); +#if CONFIG(MAINBOARD_DO_NATIVE_VGA_INIT) + sc7180_display_init(dev); +#endif }
static struct device_operations soc_ops = {
build bot (Jenkins) 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 1:
(26 comments)
https://review.coreboot.org/c/coreboot/+/39615/1/src/soc/qualcomm/sc7180/dis... File src/soc/qualcomm/sc7180/display/mdss.c:
https://review.coreboot.org/c/coreboot/+/39615/1/src/soc/qualcomm/sc7180/dis... PS1, Line 30: *pinfo); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39615/1/src/soc/qualcomm/sc7180/dis... PS1, Line 70: uint32_t *left_pipe, uint32_t *right_pipe) code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39615/1/src/soc/qualcomm/sc7180/dis... PS1, Line 91: !pinfo->mipi.dual_dsi && code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39615/1/src/soc/qualcomm/sc7180/dis... PS1, Line 92: !pinfo->lcdc.split_display; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39615/1/src/soc/qualcomm/sc7180/dis... PS1, Line 101: *ctl0_reg_val |= BIT(12)| BIT(11) | BIT(6) | BIT(7); need consistent spacing around '|' (ctx:VxW)
https://review.coreboot.org/c/coreboot/+/39615/1/src/soc/qualcomm/sc7180/dis... PS1, Line 105: *ctl1_reg_val |= BIT(12)| BIT(7); need consistent spacing around '|' (ctx:VxW)
https://review.coreboot.org/c/coreboot/+/39615/1/src/soc/qualcomm/sc7180/dis... PS1, Line 114: *ctl0_reg_val |= BIT(0)| BIT(1) |BIT(6) | BIT(7); need consistent spacing around '|' (ctx:VxW)
https://review.coreboot.org/c/coreboot/+/39615/1/src/soc/qualcomm/sc7180/dis... PS1, Line 114: *ctl0_reg_val |= BIT(0)| BIT(1) |BIT(6) | BIT(7); need consistent spacing around '|' (ctx:WxV)
https://review.coreboot.org/c/coreboot/+/39615/1/src/soc/qualcomm/sc7180/dis... PS1, Line 116: *ctl0_reg_val |= BIT(0)| BIT(6); need consistent spacing around '|' (ctx:VxW)
https://review.coreboot.org/c/coreboot/+/39615/1/src/soc/qualcomm/sc7180/dis... PS1, Line 131: struct msm_panel_info *pinfo, code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39615/1/src/soc/qualcomm/sc7180/dis... PS1, Line 132: uint32_t pipe_base) code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39615/1/src/soc/qualcomm/sc7180/dis... PS1, Line 155: else { else should follow close brace '}'
https://review.coreboot.org/c/coreboot/+/39615/1/src/soc/qualcomm/sc7180/dis... PS1, Line 286: itp.h_back_porch; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39615/1/src/soc/qualcomm/sc7180/dis... PS1, Line 290: itp.v_back_porch) * code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39615/1/src/soc/qualcomm/sc7180/dis... PS1, Line 291: hsync_period + itp.hsync_skew; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39615/1/src/soc/qualcomm/sc7180/dis... PS1, Line 294: + itp.hsync_skew - 1; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39615/1/src/soc/qualcomm/sc7180/dis... PS1, Line 381: fetch_start = (v_total - prefetch_avail) * h_total+ h_total + 1; need consistent spacing around '+' (ctx:VxW)
https://review.coreboot.org/c/coreboot/+/39615/1/src/soc/qualcomm/sc7180/dis... PS1, Line 384: fetch_enable |= BIT(23); //TODO: this shoudld not be enabled ? 'shoudld' may be misspelled - perhaps 'should'?
https://review.coreboot.org/c/coreboot/+/39615/1/src/soc/qualcomm/sc7180/dis... PS1, Line 415: left_staging_level =BIT(24); //attach border spaces required around that '=' (ctx:WxV)
https://review.coreboot.org/c/coreboot/+/39615/1/src/soc/qualcomm/sc7180/dis... PS1, Line 416: right_staging_level=BIT(24); spaces required around that '=' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/39615/1/src/soc/qualcomm/sc7180/dis... PS1, Line 449: writel(0x00ff0000, MDP_VP_0_MIXER_1_BASE + LAYER_0_BLEND0_CONST_ALPHA(i)); line over 96 characters
https://review.coreboot.org/c/coreboot/+/39615/1/src/soc/qualcomm/sc7180/dis... PS1, Line 478: int is_main_ctl) code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39615/1/src/soc/qualcomm/sc7180/dis... PS1, Line 498: uint32_t *intf_base, uint32_t *sintf_base) code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39615/1/src/soc/qualcomm/sc7180/dis... PS1, Line 510: struct fbcon_config *fb) code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39615/1/src/soc/qualcomm/sc7180/dis... PS1, Line 558: MDP_DSC_0_BASE, true, true); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39615/1/src/soc/qualcomm/sc7180/dis... PS1, Line 608: if (pinfo->lcdc.dual_pipe && !pinfo->lcdc.dst_split){ space required before the open brace '{'
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Julius Werner, mturney mturney,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39615
to look at the new patch set (#2).
Change subject: sc7180: Add display hardware pipe line initialization [patch 3 of 3] ......................................................................
sc7180: Add display hardware pipe line initialization [patch 3 of 3]
Add sc7180 display hardware pipeline programming support and invoke the display initialization from soc_init
Change-Id: I7d5e3f1781c48759553243abeb3d694f76cd008e Signed-off-by: Vinod Polimera vpolimer@codeaurora.org --- M src/soc/qualcomm/sc7180/Kconfig M src/soc/qualcomm/sc7180/Makefile.inc A src/soc/qualcomm/sc7180/display.c A src/soc/qualcomm/sc7180/display/mdss.c A src/soc/qualcomm/sc7180/display/oem_panel.c A src/soc/qualcomm/sc7180/include/soc/display.h A src/soc/qualcomm/sc7180/include/soc/display/panel_sn65dsix6_auo_bll6xak01_dsi_video.h M src/soc/qualcomm/sc7180/soc.c 8 files changed, 1,451 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/39615/2
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).
mturney mturney has uploaded a new patch set (#4) to the change originally created by Ravi kumar. ( https://review.coreboot.org/c/coreboot/+/39615 )
Change subject: sc7180: Add display hardware pipe line initialization [patch 3 of 3] ......................................................................
sc7180: Add display hardware pipe line initialization [patch 3 of 3]
Add sc7180 display hardware pipeline programming support and invoke the display initialization from soc_init
Change-Id: I7d5e3f1781c48759553243abeb3d694f76cd008e Signed-off-by: Vinod Polimera vpolimer@codeaurora.org --- M src/soc/qualcomm/sc7180/Kconfig M src/soc/qualcomm/sc7180/Makefile.inc A src/soc/qualcomm/sc7180/display.c A src/soc/qualcomm/sc7180/display/mdss.c A src/soc/qualcomm/sc7180/display/oem_panel.c A src/soc/qualcomm/sc7180/include/soc/display.h A src/soc/qualcomm/sc7180/include/soc/display/panel_sn65dsix6_auo_bll6xak01_dsi_video.h M src/soc/qualcomm/sc7180/soc.c 8 files changed, 1,451 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/39615/4
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Julius Werner, mturney mturney, mturney mturney,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39615
to look at the new patch set (#7).
Change subject: sc7180: Add display hardware pipe line initialization [patch 3 of 3] ......................................................................
sc7180: Add display hardware pipe line initialization [patch 3 of 3]
Add sc7180 display hardware pipeline programming support and invoke the display initialization from soc_init
Change-Id: I7d5e3f1781c48759553243abeb3d694f76cd008e Signed-off-by: Vinod Polimera vpolimer@codeaurora.org --- M src/soc/qualcomm/sc7180/Kconfig M src/soc/qualcomm/sc7180/Makefile.inc A src/soc/qualcomm/sc7180/display.c A src/soc/qualcomm/sc7180/display/mdss.c A src/soc/qualcomm/sc7180/display/oem_panel.c A src/soc/qualcomm/sc7180/include/soc/display.h A src/soc/qualcomm/sc7180/include/soc/display/panel_sn65dsix6_auo_bll6xak01_dsi_video.h M src/soc/qualcomm/sc7180/soc.c 8 files changed, 1,451 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/39615/7
mturney mturney has uploaded a new patch set (#8) to the change originally created by Ravi kumar. ( https://review.coreboot.org/c/coreboot/+/39615 )
Change subject: sc7180: Add display hardware pipe line initialization [patch 3 of 3] ......................................................................
sc7180: Add display hardware pipe line initialization [patch 3 of 3]
Add sc7180 display hardware pipeline programming support and invoke the display initialization from soc_init
Change-Id: I7d5e3f1781c48759553243abeb3d694f76cd008e Signed-off-by: Vinod Polimera vpolimer@codeaurora.org --- M src/soc/qualcomm/sc7180/Kconfig M src/soc/qualcomm/sc7180/Makefile.inc A src/soc/qualcomm/sc7180/display.c A src/soc/qualcomm/sc7180/display/mdss.c A src/soc/qualcomm/sc7180/display/oem_panel.c A src/soc/qualcomm/sc7180/include/soc/display.h A src/soc/qualcomm/sc7180/include/soc/display/panel_sn65dsix6_auo_bll6xak01_dsi_video.h M src/soc/qualcomm/sc7180/soc.c 8 files changed, 1,451 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/39615/8
mturney mturney has uploaded a new patch set (#9) to the change originally created by Ravi kumar. ( https://review.coreboot.org/c/coreboot/+/39615 )
Change subject: sc7180: Add display hardware pipe line initialization [patch 3 of 3] ......................................................................
sc7180: Add display hardware pipe line initialization [patch 3 of 3]
Add sc7180 display hardware pipeline programming support and invoke the display initialization from soc_init
Change-Id: I7d5e3f1781c48759553243abeb3d694f76cd008e Signed-off-by: Vinod Polimera vpolimer@codeaurora.org --- M src/soc/qualcomm/sc7180/Kconfig M src/soc/qualcomm/sc7180/Makefile.inc A src/soc/qualcomm/sc7180/display.c A src/soc/qualcomm/sc7180/display/mdss.c A src/soc/qualcomm/sc7180/display/oem_panel.c A src/soc/qualcomm/sc7180/include/soc/display.h A src/soc/qualcomm/sc7180/include/soc/display/panel_sn65dsix6_auo_bll6xak01_dsi_video.h M src/soc/qualcomm/sc7180/soc.c 8 files changed, 1,450 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/39615/9
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Julius Werner, mturney mturney, mturney mturney,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39615
to look at the new patch set (#12).
Change subject: sc7180: Add display hardware pipe line initialization [patch 3 of 3] ......................................................................
sc7180: Add display hardware pipe line initialization [patch 3 of 3]
Add sc7180 display hardware pipeline programming support and invoke the display initialization from soc_init
Change-Id: I7d5e3f1781c48759553243abeb3d694f76cd008e Signed-off-by: Vinod Polimera vpolimer@codeaurora.org --- M src/soc/qualcomm/sc7180/Kconfig M src/soc/qualcomm/sc7180/Makefile.inc A src/soc/qualcomm/sc7180/display.c A src/soc/qualcomm/sc7180/display/mdss.c A src/soc/qualcomm/sc7180/display/oem_panel.c A src/soc/qualcomm/sc7180/include/soc/display.h A src/soc/qualcomm/sc7180/include/soc/display/panel_sn65dsix6_auo_bll6xak01_dsi_video.h M src/soc/qualcomm/sc7180/soc.c 8 files changed, 1,450 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/39615/12
Manideep Kurumella has uploaded a new patch set (#13) to the change originally created by Ravi kumar. ( https://review.coreboot.org/c/coreboot/+/39615 )
Change subject: sc7180: Add display hardware pipe line initialization [patch 3 of 3] ......................................................................
sc7180: Add display hardware pipe line initialization [patch 3 of 3]
Add sc7180 display hardware pipeline programming support and invoke the display initialization from soc_init
Change-Id: I7d5e3f1781c48759553243abeb3d694f76cd008e Signed-off-by: Vinod Polimera vpolimer@codeaurora.org --- M src/mainboard/google/trogdor/mainboard.c A src/soc/qualcomm/common/include/soc/sn65dsi86bridge.h A src/soc/qualcomm/common/sn65dsi86bridge.c M src/soc/qualcomm/sc7180/Kconfig M src/soc/qualcomm/sc7180/Makefile.inc A src/soc/qualcomm/sc7180/display.c A src/soc/qualcomm/sc7180/display/mdss.c A src/soc/qualcomm/sc7180/display/oem_panel.c A src/soc/qualcomm/sc7180/include/soc/display.h A src/soc/qualcomm/sc7180/include/soc/display/panel_sn65dsix6_auo_bll6xak01_dsi_video.h 10 files changed, 1,240 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/39615/13
build bot (Jenkins) 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 13:
(17 comments)
https://review.coreboot.org/c/coreboot/+/39615/13/src/soc/qualcomm/common/in... File src/soc/qualcomm/common/include/soc/sn65dsi86bridge.h:
https://review.coreboot.org/c/coreboot/+/39615/13/src/soc/qualcomm/common/in... PS13, Line 99: enum vstream_config{ missing space after enum definition
https://review.coreboot.org/c/coreboot/+/39615/13/src/soc/qualcomm/common/in... PS13, Line 112: enum i2c_over_aux{ missing space after enum definition
https://review.coreboot.org/c/coreboot/+/39615/13/src/soc/qualcomm/common/in... PS13, Line 113: I2C_OVER_AUX_WRITE_MOT_0 , space prohibited before that ',' (ctx:WxE)
https://review.coreboot.org/c/coreboot/+/39615/13/src/soc/qualcomm/common/sn... File src/soc/qualcomm/common/sn65dsi86bridge.c:
https://review.coreboot.org/c/coreboot/+/39615/13/src/soc/qualcomm/common/sn... PS13, Line 40: #define DP_BRIDGE_DPCD_REV 0x700 please, no space before tabs
https://review.coreboot.org/c/coreboot/+/39615/13/src/soc/qualcomm/common/sn... PS13, Line 49: #define DP_MAX_LINK_RATE 0x001 please, no space before tabs
https://review.coreboot.org/c/coreboot/+/39615/13/src/soc/qualcomm/common/sn... PS13, Line 55: #define DP_LINK_BW_1_62 0x06 please, no space before tabs
https://review.coreboot.org/c/coreboot/+/39615/13/src/soc/qualcomm/common/sn... PS13, Line 85: i2c_writeb(I2C_BUS, CHIP, SN_AUX_ADDR_15_8_REG, (dpcd_reg >> 8) & 0xFF ); space prohibited before that close parenthesis ')'
https://review.coreboot.org/c/coreboot/+/39615/13/src/soc/qualcomm/common/sn... PS13, Line 145: printk(BIOS_ERR,"No matching eDP rates in table; falling back\n"); space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/39615/13/src/soc/qualcomm/common/sn... PS13, Line 188: uint64_t pixel_clk ; space prohibited before semicolon
https://review.coreboot.org/c/coreboot/+/39615/13/src/soc/qualcomm/common/sn... PS13, Line 211: uint64_t dp_rate_mhz ; space prohibited before semicolon
https://review.coreboot.org/c/coreboot/+/39615/13/src/soc/qualcomm/common/sn... PS13, Line 273: if (!wait_us(500000,(!(i2c_readb(I2C_BUS, CHIP, SN_DPPLL_SRC_REG, &buf)) && suspect code indent for conditional statements (8, 24)
https://review.coreboot.org/c/coreboot/+/39615/13/src/soc/qualcomm/common/sn... PS13, Line 273: if (!wait_us(500000,(!(i2c_readb(I2C_BUS, CHIP, SN_DPPLL_SRC_REG, &buf)) && space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/39615/13/src/soc/qualcomm/common/sn... PS13, Line 275: printk(BIOS_ERR,"PLL lock failure"); space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/39615/13/src/soc/qualcomm/common/sn... PS13, Line 290: if (!wait_us(500000,(!(i2c_readb(I2C_BUS, CHIP, SN_ML_TX_MODE_REG, &buf)) && space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/39615/13/src/soc/qualcomm/common/sn... PS13, Line 292: printk(BIOS_ERR,"Link training failed"); space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/39615/13/src/soc/qualcomm/common/sn... PS13, Line 358: i2c_write_field(I2C_BUS, CHIP, SN_SSC_CONFIG_REG, MIN(data,3), 3, 4); space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/39615/13/src/soc/qualcomm/common/sn... PS13, Line 410: } adding a line without newline at end of file
Manideep Kurumella has uploaded a new patch set (#14) to the change originally created by Ravi kumar. ( https://review.coreboot.org/c/coreboot/+/39615 )
Change subject: sc7180: Add display hardware pipe line initialization [patch 3 of 3] ......................................................................
sc7180: Add display hardware pipe line initialization [patch 3 of 3]
Add sc7180 display hardware pipeline programming support and invoke the display initialization from soc_init
Change-Id: I7d5e3f1781c48759553243abeb3d694f76cd008e Signed-off-by: Vinod Polimera vpolimer@codeaurora.org --- M src/soc/qualcomm/sc7180/Kconfig M src/soc/qualcomm/sc7180/Makefile.inc A src/soc/qualcomm/sc7180/display.c A src/soc/qualcomm/sc7180/display/mdss.c A src/soc/qualcomm/sc7180/display/oem_panel.c A src/soc/qualcomm/sc7180/include/soc/display.h A src/soc/qualcomm/sc7180/include/soc/display/panel_sn65dsix6_auo_bll6xak01_dsi_video.h M src/soc/qualcomm/sc7180/soc.c 8 files changed, 1,450 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/39615/14
Manideep Kurumella has uploaded a new patch set (#15) to the change originally created by Ravi kumar. ( https://review.coreboot.org/c/coreboot/+/39615 )
Change subject: sc7180: Add display hardware pipe line initialization [patch 3 of 3] ......................................................................
sc7180: Add display hardware pipe line initialization [patch 3 of 3]
Add sc7180 display hardware pipeline programming support and invoke the display initialization from soc_init
Change-Id: I7d5e3f1781c48759553243abeb3d694f76cd008e Signed-off-by: Vinod Polimera vpolimer@codeaurora.org --- M src/mainboard/google/trogdor/mainboard.c A src/soc/qualcomm/common/include/soc/sn65dsi86bridge.h A src/soc/qualcomm/common/sn65dsi86bridge.c M src/soc/qualcomm/sc7180/Kconfig M src/soc/qualcomm/sc7180/Makefile.inc A src/soc/qualcomm/sc7180/display.c A src/soc/qualcomm/sc7180/display/mdss.c A src/soc/qualcomm/sc7180/display/oem_panel.c A src/soc/qualcomm/sc7180/include/soc/display.h A src/soc/qualcomm/sc7180/include/soc/display/panel_sn65dsix6_auo_bll6xak01_dsi_video.h 10 files changed, 1,240 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/39615/15
build bot (Jenkins) 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 15:
(17 comments)
https://review.coreboot.org/c/coreboot/+/39615/15/src/soc/qualcomm/common/in... File src/soc/qualcomm/common/include/soc/sn65dsi86bridge.h:
https://review.coreboot.org/c/coreboot/+/39615/15/src/soc/qualcomm/common/in... PS15, Line 99: enum vstream_config{ missing space after enum definition
https://review.coreboot.org/c/coreboot/+/39615/15/src/soc/qualcomm/common/in... PS15, Line 112: enum i2c_over_aux{ missing space after enum definition
https://review.coreboot.org/c/coreboot/+/39615/15/src/soc/qualcomm/common/in... PS15, Line 113: I2C_OVER_AUX_WRITE_MOT_0 , space prohibited before that ',' (ctx:WxE)
https://review.coreboot.org/c/coreboot/+/39615/15/src/soc/qualcomm/common/sn... File src/soc/qualcomm/common/sn65dsi86bridge.c:
https://review.coreboot.org/c/coreboot/+/39615/15/src/soc/qualcomm/common/sn... PS15, Line 40: #define DP_BRIDGE_DPCD_REV 0x700 please, no space before tabs
https://review.coreboot.org/c/coreboot/+/39615/15/src/soc/qualcomm/common/sn... PS15, Line 49: #define DP_MAX_LINK_RATE 0x001 please, no space before tabs
https://review.coreboot.org/c/coreboot/+/39615/15/src/soc/qualcomm/common/sn... PS15, Line 55: #define DP_LINK_BW_1_62 0x06 please, no space before tabs
https://review.coreboot.org/c/coreboot/+/39615/15/src/soc/qualcomm/common/sn... PS15, Line 85: i2c_writeb(I2C_BUS, CHIP, SN_AUX_ADDR_15_8_REG, (dpcd_reg >> 8) & 0xFF ); space prohibited before that close parenthesis ')'
https://review.coreboot.org/c/coreboot/+/39615/15/src/soc/qualcomm/common/sn... PS15, Line 145: printk(BIOS_ERR,"No matching eDP rates in table; falling back\n"); space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/39615/15/src/soc/qualcomm/common/sn... PS15, Line 188: uint64_t pixel_clk ; space prohibited before semicolon
https://review.coreboot.org/c/coreboot/+/39615/15/src/soc/qualcomm/common/sn... PS15, Line 211: uint64_t dp_rate_mhz ; space prohibited before semicolon
https://review.coreboot.org/c/coreboot/+/39615/15/src/soc/qualcomm/common/sn... PS15, Line 273: if (!wait_us(500000,(!(i2c_readb(I2C_BUS, CHIP, SN_DPPLL_SRC_REG, &buf)) && suspect code indent for conditional statements (8, 24)
https://review.coreboot.org/c/coreboot/+/39615/15/src/soc/qualcomm/common/sn... PS15, Line 273: if (!wait_us(500000,(!(i2c_readb(I2C_BUS, CHIP, SN_DPPLL_SRC_REG, &buf)) && space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/39615/15/src/soc/qualcomm/common/sn... PS15, Line 275: printk(BIOS_ERR,"PLL lock failure"); space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/39615/15/src/soc/qualcomm/common/sn... PS15, Line 290: if (!wait_us(500000,(!(i2c_readb(I2C_BUS, CHIP, SN_ML_TX_MODE_REG, &buf)) && space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/39615/15/src/soc/qualcomm/common/sn... PS15, Line 292: printk(BIOS_ERR,"Link training failed"); space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/39615/15/src/soc/qualcomm/common/sn... PS15, Line 358: i2c_write_field(I2C_BUS, CHIP, SN_SSC_CONFIG_REG, MIN(data,3), 3, 4); space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/39615/15/src/soc/qualcomm/common/sn... PS15, Line 410: } adding a line without newline at end of file
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Julius Werner, mturney mturney, mturney mturney,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39615
to look at the new patch set (#16).
Change subject: sc7180: Add display hardware pipe line initialization [patch 3 of 3] ......................................................................
sc7180: Add display hardware pipe line initialization [patch 3 of 3]
Add sc7180 display hardware pipeline programming support and invoke the display initialization from soc_init
Change-Id: I7d5e3f1781c48759553243abeb3d694f76cd008e Signed-off-by: Vinod Polimera vpolimer@codeaurora.org --- M src/mainboard/google/trogdor/mainboard.c A src/soc/qualcomm/common/include/soc/sn65dsi86bridge.h A src/soc/qualcomm/common/sn65dsi86bridge.c M src/soc/qualcomm/sc7180/Kconfig M src/soc/qualcomm/sc7180/Makefile.inc A src/soc/qualcomm/sc7180/display.c A src/soc/qualcomm/sc7180/display/mdss.c A src/soc/qualcomm/sc7180/display/oem_panel.c A src/soc/qualcomm/sc7180/include/soc/display.h A src/soc/qualcomm/sc7180/include/soc/display/panel_sn65dsix6_auo_bll6xak01_dsi_video.h 10 files changed, 1,241 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/39615/16
build bot (Jenkins) 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 16:
(2 comments)
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 187: uint64_t pixel_clk ; space prohibited before semicolon
https://review.coreboot.org/c/coreboot/+/39615/16/src/soc/qualcomm/common/sn... PS16, Line 272: if (!wait_us(500000, suspect code indent for conditional statements (8, 24)
HAOUAS Elyes 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 16:
(3 comments)
Please include only what you use.
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
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
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
please clean also the other files
build bot (Jenkins) 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 17:
(2 comments)
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 187: uint64_t pixel_clk ; space prohibited before semicolon
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/sn... PS17, Line 272: if (!wait_us(500000, suspect code indent for conditional statements (8, 24)
build bot (Jenkins) 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 18:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39615/18/src/soc/qualcomm/common/sn... File src/soc/qualcomm/common/sn65dsi86bridge.c:
https://review.coreboot.org/c/coreboot/+/39615/18/src/soc/qualcomm/common/sn... PS18, Line 187: uint64_t pixel_clk ; space prohibited before semicolon
https://review.coreboot.org/c/coreboot/+/39615/18/src/soc/qualcomm/common/sn... PS18, Line 272: if (!wait_us(500000, suspect code indent for conditional statements (8, 24)
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Julius Werner, mturney mturney, mturney mturney,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39615
to look at the new patch set (#19).
Change subject: sc7180: Add display hardware pipe line initialization [patch 3 of 3] ......................................................................
sc7180: Add display hardware pipe line initialization [patch 3 of 3]
Add sc7180 display hardware pipeline programming support and invoke the display initialization from soc_init
Change-Id: I7d5e3f1781c48759553243abeb3d694f76cd008e Signed-off-by: Vinod Polimera vpolimer@codeaurora.org --- M src/mainboard/google/trogdor/mainboard.c A src/soc/qualcomm/common/include/soc/sn65dsi86bridge.h A src/soc/qualcomm/common/sn65dsi86bridge.c M src/soc/qualcomm/sc7180/Kconfig M src/soc/qualcomm/sc7180/Makefile.inc A src/soc/qualcomm/sc7180/display.c A src/soc/qualcomm/sc7180/display/mdss.c A src/soc/qualcomm/sc7180/display/oem_panel.c A src/soc/qualcomm/sc7180/include/soc/display.h A src/soc/qualcomm/sc7180/include/soc/display/panel_sn65dsix6_auo_bll6xak01_dsi_video.h 10 files changed, 1,241 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/39615/19
build bot (Jenkins) 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 19:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39615/19/src/soc/qualcomm/common/sn... File src/soc/qualcomm/common/sn65dsi86bridge.c:
https://review.coreboot.org/c/coreboot/+/39615/19/src/soc/qualcomm/common/sn... PS19, Line 187: uint64_t pixel_clk ; space prohibited before semicolon
https://review.coreboot.org/c/coreboot/+/39615/19/src/soc/qualcomm/common/sn... PS19, Line 272: if (!wait_us(500000, suspect code indent for conditional statements (8, 24)
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 19:
(34 comments)
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. pack this and the stuff you added in mainboard_init() into a configure_display() or something like that).
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. Please submit everything going in that directory in a separate patch from the SoC and mainboard stuff.
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
/* SPDX-License-Identifier: GPL-2.0-only */
now.
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).
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. SN65_SEL_12MHZ or something like that. (You also don't really need to put all this register-specific stuff in the header. If you just define them in the .c file, it's okay if they're not namespaced.)
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 a value to each field and not just rely on them counting up from 0, e.g.:
SEL_12MHZ = 0, SEL_19MHZ = 1, SEL_26MHZ = 2, ...
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.
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++?
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... this is *waaaay* too slow. The datasheet says the AUX_CMD_SEND bit gets cleared when the transaction is done, so you could just poll for that.
However, I think it would be even better if you just used the "direct method" they describe in the data sheet: if I understand this correctly, you can just pick a random I2C chip address (e.g. (CHIP + 1)), write it to I2C_ADDR_CLAIM1, set the I2C_CLAIM1_EN bit, and then any I2C requests you send to that address will automatically get tunneled to the DP AUX channel. Using this here should be much more efficient than the "indirect method" you implemented (because it's only one I2C transfer per actual DPCD transfer, not 7+).
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.
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?
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, ...).
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/sn... PS17, Line 273: ( Don't really need the outermost parenthesis here.
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 complaining about.
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.
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()
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/sn... PS17, Line 314: if (buf & HPD_DISABLE) This should be
if (val == 0 && (buf & HPD_DISABLE))
since if val != 0, buf may not contain valid data.
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
if (wait_ms(360, sn65dsi86_bridge_get_plug_in_status())) return;
Also, it would be good to print a warning if HPD was not detected after the timeout.
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.
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.
I don't really understand why you need to pass hardcoded panel information in from the mainboard here. This MIPI-to-eDP bridge connects to an eDP panel, which (like all eDP panels) should have an EDID structure describing panel parameters that you can read out via the AUX channel. Please see the SN65DSI86 datasheet for information about how to read out the EDID. (They describe both a "direct" method via the I2C_ADDR_CLAIMx registers and an "indirect" method via packet passthrough. Direct is probably more efficient but either of them should work.) After you have the raw EDID data, you can use the decode_edid() function to extract a struct edid out of it which contains all these parameters. Like I mentioned before, this should all work very similar to the src/drivers/parade/ps8640/ps8640.c driver (you can see it used in src/mainboard/google/oak/mainboard.c, for example).
For the num_of_lanes field I would just pass that as a raw 'int' parameter.
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 parameter.
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. Bus number should be passed in as a parameter. Chip address can also be passed in or you could just hardcode it for now (since we're unlikely to ever need the address selection).
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. Why here? This function belongs in the bridge driver, and needs to be called from mainboard code.
The bridge driver and the SoC display code need to be 100% completely independent and not share any headers other than common stuff like struct edid. The mainboard code needs to tie them together -- first initializing the bridge, using it to fetch the EDID, then passing that EDID (and whatever other info may be necessary, e.g. number of MIPI lanes or whatever) to the SoC display initialization code.
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/sc7180/di... PS17, Line 79: 0xA1 This should be written as
(EDID_I2C_ADDR << 1) | 0x1
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). If this wait is related to the panel coming up, it should not be in bridge code.
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. You should write the DPCD interface like this as well. (You can just write I2C_CLAIM_ADDR_EN1 once in the init function and then keep using it.)
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 a few extra fields that are probably easier to pass manually). And all this information needs to come in from the mainboard. The code under soc/qualcomm/sc7180 should not contain any information about a specific panel from a specific vendor.
Basically, the words "panel" or "oem" should probably not appear anywhere in the code under soc/qualcomm/sc7180. (If there are specific problems with designing it this way, please let me know so we can discuss them in detail. In general, for eDP panels the EDID should always be enough to describe anything the display stack should need to know about the panel. That's how it has worked on all our other platforms before.
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... do we have any kinds of panels other than DSI? Isn't this just a driver for a MIPI DSI controller? Same with the pinfo->type where MIPI_VIDEO_PANEL is the only type we'll presumably ever have... please do not add complication that we'll never have a use for.)
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 structures for all of this.
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.
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.
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.
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 as parameters from the mainboard to the display init function. For example, look how this is done in https://review.coreboot.org/cgit/coreboot.git/tree/src/mainboard/google/oak/... -- they just pass the amount of lanes, a video format identifier, and a couple of flags to handle other stuff to the SoC display function, and that's it. You shouldn't really need much more than that either. (If you want I'm okay with packaging this stuff into a structure too, but it should only contain the few fields that you really need, not all this stuff that will always be zeroes in practice. And nothing that's redundant with struct edid.)
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. I've never seen any other eDP panel that needs so much hardcoded stuff, eDP panels should self-report everything needed through the EDID. That's how every other platform I have seen works.
I don't see the kernel having anything like this hardcoded anywhere. If they don't need it to get the display running, why do we?
build bot (Jenkins) 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 20:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39615/20/src/soc/qualcomm/common/sn... File src/soc/qualcomm/common/sn65dsi86bridge.c:
https://review.coreboot.org/c/coreboot/+/39615/20/src/soc/qualcomm/common/sn... PS20, Line 187: uint64_t pixel_clk ; space prohibited before semicolon
https://review.coreboot.org/c/coreboot/+/39615/20/src/soc/qualcomm/common/sn... PS20, Line 272: if (!wait_us(500000, suspect code indent for conditional statements (8, 24)
build bot (Jenkins) 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 21:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39615/21/src/soc/qualcomm/common/sn... File src/soc/qualcomm/common/sn65dsi86bridge.c:
https://review.coreboot.org/c/coreboot/+/39615/21/src/soc/qualcomm/common/sn... PS21, Line 187: uint64_t pixel_clk ; space prohibited before semicolon
https://review.coreboot.org/c/coreboot/+/39615/21/src/soc/qualcomm/common/sn... PS21, Line 272: if (!wait_us(500000, suspect code indent for conditional statements (8, 24)
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Julius Werner, mturney mturney, mturney mturney,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39615
to look at the new patch set (#22).
Change subject: sc7180: Add display hardware pipe line initialization ......................................................................
sc7180: Add display hardware pipe line initialization
Add sc7180 display hardware pipeline programming support and invoke the display initialization from soc_init.
Changes in V1: - added display init required check. - added edid read function using i2c communication. - added sn65dsi86 bridge driver to init bridge. - moved display initialization to mainboard file.
Changes in V2: - moved diplay init sequence to mainboard file - moved edid read function to bridge driver. - calculated timing paramters using edid parameters. - removed command mode config code. - moved bridge driver to drivers/ti. - seperated out bridge and soc code with mainboard file as interface.
Change-Id: I7d5e3f1781c48759553243abeb3d694f76cd008e Signed-off-by: Vinod Polimera vpolimer@codeaurora.org --- M src/mainboard/google/trogdor/Kconfig M src/mainboard/google/trogdor/mainboard.c M src/soc/qualcomm/sc7180/Kconfig M src/soc/qualcomm/sc7180/Makefile.inc A src/soc/qualcomm/sc7180/display/mdss.c 5 files changed, 287 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/39615/22
build bot (Jenkins) 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:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39615/22/src/mainboard/google/trogd... File src/mainboard/google/trogdor/mainboard.c:
https://review.coreboot.org/c/coreboot/+/39615/22/src/mainboard/google/trogd... PS22, Line 94: if(ret) space required before the open parenthesis '('
https://review.coreboot.org/c/coreboot/+/39615/22/src/mainboard/google/trogd... PS22, Line 119: return ; space prohibited before semicolon
https://review.coreboot.org/c/coreboot/+/39615/22/src/mainboard/google/trogd... PS22, Line 129: else else should follow close brace '}'
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
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Julius Werner, mturney mturney, mturney mturney,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39615
to look at the new patch set (#23).
Change subject: sc7180: Add display hardware pipe line initialization ......................................................................
sc7180: Add display hardware pipe line initialization
Add sc7180 display hardware pipeline programming support and invoke the display initialization from soc_init.
Changes in V1: - added display init required check. - added edid read function using i2c communication. - added sn65dsi86 bridge driver to init bridge. - moved display initialization to mainboard file.
Changes in V2: - moved diplay init sequence to mainboard file - moved edid read function to bridge driver. - calculated timing paramters using edid parameters. - removed command mode config code. - moved bridge driver to drivers/ti. - seperated out bridge and soc code with mainboard file as interface.
Change-Id: I7d5e3f1781c48759553243abeb3d694f76cd008e Signed-off-by: Vinod Polimera vpolimer@codeaurora.org --- M src/mainboard/google/trogdor/Kconfig M src/mainboard/google/trogdor/mainboard.c M src/soc/qualcomm/sc7180/Kconfig M src/soc/qualcomm/sc7180/Makefile.inc A src/soc/qualcomm/sc7180/display/mdss.c 5 files changed, 286 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/39615/23
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 Set 25:
(17 comments)
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... File src/mainboard/google/trogdor/Kconfig:
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, 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 to make the bridge driver come earlier in the patch series.
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... File src/mainboard/google/trogdor/mainboard.c:
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, 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.
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, 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 it.
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, Line 73: gpio_configure(GPIO(14), GPIO_FUNC_GPIO, GPIO_NO_PULL, This is only correct for Trogdor rev0, for later boards (e.g. Trogdor rev1 or any Lazor) we changed this to GPIO(104). Please define this in board.h (like GPIO_H1_AP_INT is also different for REV0 and others there).
In fact, it's probably best to define all of these in board.h, even the ones that are the same everywhere right now could still change later.
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, 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) for all others. We only have a special Kconfig option for Trogdor-rev0 (for other reasons), so you would use board_id() to decide this one. Write it as
#define GPIO_EN_PP3300_DX_EDP ((CONFIG(BOARD_GOOGLE_TROGDOR) && board_id() < 2) || (CONFIG(BOARD_GOOGLE_LAZOR) && board_id() < 1) ? GPIO(106) : GPIO(30))
in board.h.
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, Line 87: uint8_t num_of_lanes, uint8_t bpp I think you can just hardcode these in this function, rather than passing them in from another function in the same file.
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, Line 94: if (ret) nit: just write
if (mdss_dsi_config(...)) return;
no need for a 'ret'.
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, Line 97: ctrl_mode = read32(&dsi0->ctrl); Let's not open-code this in the mainboard file... create a mdss_dsi_set_command_mode(int enable) function or something like that for this.
(I'm a bit confused why this is necessary, too... does the bridge care what the DSI controller is doing during its init function? Can't we just turn the bridge on first, then initialize the DSI?)
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, Line 101: mdelay(500); Why do we need this delay? 500ms is very long.
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, Line 118: 0x2 Please make named constants for BRIDGE_BUS and BRIDGE_CHIP at the top of this file.
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, Line 123: edid_set_framebuffer_bits_per_pixel(&ed, You don't really need to call this since it is already the default. (However, if you do want to call it, you should hardcode the 32 here rather than just using the parameter that was set by the default again.)
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 97: mdelay(100);
I dont find any slave address for the DPCD read/write to implement using direct method.
Sorry, I thought this was possible but I guess I didn't quite understand the difference between the I2C AUX channel and Native DPCD. Looks like you're right and this is the only way.
https://review.coreboot.org/c/coreboot/+/39615/25/src/soc/qualcomm/sc7180/Ma... File src/soc/qualcomm/sc7180/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/39615/25/src/soc/qualcomm/sc7180/Ma... PS25, Line 65: ramstage-$(CONFIG_MAINBOARD_DO_NATIVE_VGA_INIT) += display/dsi.c Please put the Makefile lines for each file together with the patch that is adding that file.
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 80: mdelay(200);
this is required for panel coming up. Moved it before edid read call.
Ack. (200ms is a bit long though, does the panel really need that much?)
https://review.coreboot.org/c/coreboot/+/39615/25/src/soc/qualcomm/sc7180/di... File src/soc/qualcomm/sc7180/display/mdss.c:
https://review.coreboot.org/c/coreboot/+/39615/25/src/soc/qualcomm/sc7180/di... PS25, Line 16: int mdp_dsi_cmd_off(void); You should never need to declare prototypes in .c files. If these functions are supposed to be externally accessible, they belong in a .h file. If they're not, they should be 'static'.
https://review.coreboot.org/c/coreboot/+/39615/25/src/soc/qualcomm/sc7180/di... PS25, Line 156: 0x00000003 Where do all these magic numbers come from? This at least needs some comments.
https://review.coreboot.org/c/coreboot/+/39615/25/src/soc/qualcomm/sc7180/di... PS25, Line 159: MDP_VBIF_RT_BASE + 0x568 Don't do this, define a struct overlay for these registers like you have for the others.
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 Set 26:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... File src/mainboard/google/trogdor/mainboard.c:
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, Line 87: uint8_t num_of_lanes, uint8_t bpp
I think you can just hardcode these in this function, rather than passing them in from another funct […]
edit: also, let's name the 'bpp' here 'dsi_bpp' or something like that to differentiate it from the other kinds of bpp we have (e.g. framebuffer_bpp are 32, not 24).
Vinod 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 26:
(64 comments)
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... File src/mainboard/google/trogdor/Kconfig:
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, 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
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/25/src/mainboard/google/trogd... File src/mainboard/google/trogdor/mainboard.c:
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, 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
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, 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
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, 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
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, 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
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, 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
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, Line 94: if (ret)
nit: just write […]
Done
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, Line 97: ctrl_mode = read32(&dsi0->ctrl);
Let's not open-code this in the mainboard file... […]
Done
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, Line 101: mdelay(500);
Why do we need this delay? 500ms is very long.
Done
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, Line 118: 0x2
Please make named constants for BRIDGE_BUS and BRIDGE_CHIP at the top of this file.
Done
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, Line 123: edid_set_framebuffer_bits_per_pixel(&ed,
You don't really need to call this since it is already the default. […]
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... […]
Done
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);
Ack. […]
Yes, panel is not up below 200ms.
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 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
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: 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... 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/25/src/soc/qualcomm/sc7180/di... File src/soc/qualcomm/sc7180/display/mdss.c:
https://review.coreboot.org/c/coreboot/+/39615/25/src/soc/qualcomm/sc7180/di... PS25, Line 16: int mdp_dsi_cmd_off(void);
You should never need to declare prototypes in .c files. […]
Done
https://review.coreboot.org/c/coreboot/+/39615/25/src/soc/qualcomm/sc7180/di... PS25, 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
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
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Julius Werner, mturney mturney, mturney mturney,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39615
to look at the new patch set (#28).
Change subject: sc7180: Add display hardware pipe line initialization ......................................................................
sc7180: Add display hardware pipe line initialization
Add sc7180 display hardware pipeline programming support and invoke the display initialization from soc_init.
Changes in V1: - added display init required check. - added edid read function using i2c communication. - added sn65dsi86 bridge driver to init bridge. - moved display initialization to mainboard file.
Changes in V2: - moved diplay init sequence to mainboard file - moved edid read function to bridge driver. - calculated timing paramters using edid parameters. - removed command mode config code. - moved bridge driver to drivers/ti. - seperated out bridge and soc code with mainboard file as interface.
Change-Id: I7d5e3f1781c48759553243abeb3d694f76cd008e Signed-off-by: Vinod Polimera vpolimer@codeaurora.org --- M src/mainboard/google/trogdor/Kconfig M src/mainboard/google/trogdor/board.h M src/mainboard/google/trogdor/mainboard.c M src/soc/qualcomm/sc7180/Kconfig M src/soc/qualcomm/sc7180/Makefile.inc A src/soc/qualcomm/sc7180/display/mdss.c 6 files changed, 264 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/39615/28
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39615 )
Change subject: sc7180: Add display hardware pipe line initialization ......................................................................
Patch Set 28:
(6 comments)
https://review.coreboot.org/c/coreboot/+/39615/28/src/mainboard/google/trogd... File src/mainboard/google/trogdor/board.h:
https://review.coreboot.org/c/coreboot/+/39615/28/src/mainboard/google/trogd... PS28, Line 22: (CONFIG(BOARD_GOOGLE_LAZOR) && board_id() < 1) ? \ code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39615/28/src/mainboard/google/trogd... PS28, Line 22: (CONFIG(BOARD_GOOGLE_LAZOR) && board_id() < 1) ? \ please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/39615/28/src/mainboard/google/trogd... PS28, Line 23: GPIO(106) : GPIO(30)) code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39615/28/src/mainboard/google/trogd... PS28, Line 23: GPIO(106) : GPIO(30)) please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/39615/28/src/mainboard/google/trogd... File src/mainboard/google/trogdor/mainboard.c:
https://review.coreboot.org/c/coreboot/+/39615/28/src/mainboard/google/trogd... PS28, Line 85: if(mdss_dsi_config(edid, lanes, dsi_bpp)) space required before the open parenthesis '('
https://review.coreboot.org/c/coreboot/+/39615/28/src/soc/qualcomm/sc7180/di... File src/soc/qualcomm/sc7180/display/mdss.c:
https://review.coreboot.org/c/coreboot/+/39615/28/src/soc/qualcomm/sc7180/di... PS28, Line 147: /* VBIF QOS Rempaper defualt hw settings for all read/write clients */ 'defualt' may be misspelled - perhaps 'default'?
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 Set 28:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... File src/mainboard/google/trogdor/Kconfig:
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, Line 21: select DRIVERS_TI_SN65DSI86BRIDGE
Done
I don't know why you said "Done" here, did you do anything? The patch order is still wrong, the bridge driver patch needs to be ordered *before* this patch here.
https://review.coreboot.org/c/coreboot/+/39615/28/src/mainboard/google/trogd... File src/mainboard/google/trogdor/board.h:
https://review.coreboot.org/c/coreboot/+/39615/28/src/mainboard/google/trogd... PS28, Line 21: (CONFIG(BOARD_GOOGLE_TROGDOR) && board_id() < 2) || \ : (CONFIG(BOARD_GOOGLE_LAZOR) && board_id() < 1) ? \ As discussed over email this was wrong from me -- it just needs to be
(CONFIG(TROGDOR_REV0) ? GPIO(106) : GPIO(30))
https://review.coreboot.org/c/coreboot/+/39615/28/src/mainboard/google/trogd... File src/mainboard/google/trogdor/mainboard.c:
https://review.coreboot.org/c/coreboot/+/39615/28/src/mainboard/google/trogd... PS28, Line 60: uint8_t enable Do we need this enable? If it's only ever going to be 1, just take it out (maybe get rid of the whole function because there's just one line left then, just inline that in display_startup()).
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39615 )
Change subject: sc7180: Add display hardware pipe line initialization ......................................................................
Patch Set 29:
(6 comments)
https://review.coreboot.org/c/coreboot/+/39615/29/src/mainboard/google/trogd... File src/mainboard/google/trogdor/board.h:
https://review.coreboot.org/c/coreboot/+/39615/29/src/mainboard/google/trogd... PS29, Line 20: (CONFIG(BOARD_GOOGLE_LAZOR) && board_id() < 1) ? \ code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39615/29/src/mainboard/google/trogd... PS29, Line 20: (CONFIG(BOARD_GOOGLE_LAZOR) && board_id() < 1) ? \ please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/39615/29/src/mainboard/google/trogd... PS29, Line 21: GPIO(106) : GPIO(30)) code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39615/29/src/mainboard/google/trogd... PS29, Line 21: GPIO(106) : GPIO(30)) please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/39615/29/src/mainboard/google/trogd... File src/mainboard/google/trogdor/mainboard.c:
https://review.coreboot.org/c/coreboot/+/39615/29/src/mainboard/google/trogd... PS29, Line 85: if(mdss_dsi_config(edid, lanes, dsi_bpp)) space required before the open parenthesis '('
https://review.coreboot.org/c/coreboot/+/39615/29/src/soc/qualcomm/sc7180/di... File src/soc/qualcomm/sc7180/display/mdss.c:
https://review.coreboot.org/c/coreboot/+/39615/29/src/soc/qualcomm/sc7180/di... PS29, Line 147: /* VBIF QOS Rempaper defualt hw settings for all read/write clients */ 'defualt' may be misspelled - perhaps 'default'?
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39615 )
Change subject: sc7180: Add display hardware pipe line initialization ......................................................................
Patch Set 30:
(6 comments)
https://review.coreboot.org/c/coreboot/+/39615/30/src/mainboard/google/trogd... File src/mainboard/google/trogdor/board.h:
https://review.coreboot.org/c/coreboot/+/39615/30/src/mainboard/google/trogd... PS30, Line 20: (CONFIG(BOARD_GOOGLE_LAZOR) && board_id() < 1) ? \ code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39615/30/src/mainboard/google/trogd... PS30, Line 20: (CONFIG(BOARD_GOOGLE_LAZOR) && board_id() < 1) ? \ please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/39615/30/src/mainboard/google/trogd... PS30, Line 21: GPIO(106) : GPIO(30)) code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39615/30/src/mainboard/google/trogd... PS30, Line 21: GPIO(106) : GPIO(30)) please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/39615/30/src/mainboard/google/trogd... File src/mainboard/google/trogdor/mainboard.c:
https://review.coreboot.org/c/coreboot/+/39615/30/src/mainboard/google/trogd... PS30, Line 85: if(mdss_dsi_config(edid, lanes, dsi_bpp)) space required before the open parenthesis '('
https://review.coreboot.org/c/coreboot/+/39615/30/src/soc/qualcomm/sc7180/di... File src/soc/qualcomm/sc7180/display/mdss.c:
https://review.coreboot.org/c/coreboot/+/39615/30/src/soc/qualcomm/sc7180/di... PS30, Line 147: /* VBIF QOS Rempaper defualt hw settings for all read/write clients */ 'defualt' may be misspelled - perhaps 'default'?
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Julius Werner, mturney mturney, mturney mturney,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39615
to look at the new patch set (#31).
Change subject: sc7180: Add display hardware pipe line initialization ......................................................................
sc7180: Add display hardware pipe line initialization
Add sc7180 display hardware pipeline programming support and invoke the display initialization from soc_init.
Changes in V1: - added display init required check. - added edid read function using i2c communication. - added sn65dsi86 bridge driver to init bridge. - moved display initialization to mainboard file.
Changes in V2: - moved diplay init sequence to mainboard file - moved edid read function to bridge driver. - calculated timing paramters using edid parameters. - removed command mode config code. - moved bridge driver to drivers/ti. - seperated out bridge and soc code with mainboard file as interface.
Changes in V3: - add GPIO selection at runtime based on boardid. - add vbif register struct overlay.
Changes in V4: - update gpio config for lazor board.
Change-Id: I7d5e3f1781c48759553243abeb3d694f76cd008e Signed-off-by: Vinod Polimera vpolimer@codeaurora.org --- M src/mainboard/google/trogdor/board.h M src/mainboard/google/trogdor/mainboard.c M src/soc/qualcomm/sc7180/Kconfig M src/soc/qualcomm/sc7180/Makefile.inc A src/soc/qualcomm/sc7180/display/mdss.c 5 files changed, 257 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/39615/31
Vinod 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 31:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... File src/mainboard/google/trogdor/Kconfig:
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, Line 21: select DRIVERS_TI_SN65DSI86BRIDGE
I don't know why you said "Done" here, did you do anything? The patch order is still wrong, the brid […]
In the internal patch set submitted, i have updated the order. Not sure , it got missed here.
https://review.coreboot.org/c/coreboot/+/39615/28/src/mainboard/google/trogd... File src/mainboard/google/trogdor/board.h:
https://review.coreboot.org/c/coreboot/+/39615/28/src/mainboard/google/trogd... PS28, Line 21: (CONFIG(BOARD_GOOGLE_TROGDOR) && board_id() < 2) || \ : (CONFIG(BOARD_GOOGLE_LAZOR) && board_id() < 1) ? \
As discussed over email this was wrong from me -- it just needs to be […]
Done
https://review.coreboot.org/c/coreboot/+/39615/28/src/mainboard/google/trogd... File src/mainboard/google/trogdor/mainboard.c:
https://review.coreboot.org/c/coreboot/+/39615/28/src/mainboard/google/trogd... PS28, Line 60: uint8_t enable
Do we need this enable? If it's only ever going to be 1, just take it out (maybe get rid of the whol […]
Done
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 Set 32:
(5 comments)
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... File src/mainboard/google/trogdor/Kconfig:
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, Line 21: select DRIVERS_TI_SN65DSI86BRIDGE
In the internal patch set submitted, i have updated the order. Not sure , it got missed here.
Well, I mean it's still not fixed. Please talk to Ravi or whoever is in charge of uploading these here and make sure they get ordered correctly the next time.
https://review.coreboot.org/c/coreboot/+/39615/32/src/mainboard/google/trogd... File src/mainboard/google/trogdor/mainboard.c:
https://review.coreboot.org/c/coreboot/+/39615/32/src/mainboard/google/trogd... PS32, Line 8: #include "board.h" Please order the headers with "double quotes" below all the headers with <angled brackets> with a blank line in between. Also, please order headers alphabetically (i.e. most of these should come above <soc/qupv3_config.h>).
https://review.coreboot.org/c/coreboot/+/39615/32/src/mainboard/google/trogd... PS32, Line 93: 200 Note: This number likely needs an update for Pompom (see issuetracker.google.com/issues/161373813). Depending on whether we get the question of exactly how much resolved in time, we can either slip that into this patch or fix it as a follow-up.
https://review.coreboot.org/c/coreboot/+/39615/25/src/soc/qualcomm/sc7180/Ma... File src/soc/qualcomm/sc7180/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/39615/25/src/soc/qualcomm/sc7180/Ma... PS25, Line 65: ramstage-$(CONFIG_MAINBOARD_DO_NATIVE_VGA_INIT) += display/dsi.c
Please put the Makefile lines for each file together with the patch that is adding that file.
Done
https://review.coreboot.org/c/coreboot/+/39615/25/src/soc/qualcomm/sc7180/di... File src/soc/qualcomm/sc7180/display/mdss.c:
https://review.coreboot.org/c/coreboot/+/39615/25/src/soc/qualcomm/sc7180/di... PS25, Line 156: 0x00000003
Where do all these magic numbers come from? This at least needs some comments.
*ping* (still unresolved)
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 Set 33:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39615/32/src/mainboard/google/trogd... File src/mainboard/google/trogdor/mainboard.c:
https://review.coreboot.org/c/coreboot/+/39615/32/src/mainboard/google/trogd... PS32, Line 93: 200
Note: This number likely needs an update for Pompom (see issuetracker.google.com/issues/161373813). […]
Update: Pompom guys confirmed that 250ms is enough to fix their issues, so please just update this number to that for the next patch iteration.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Julius Werner, mturney mturney, mturney mturney,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39615
to look at the new patch set (#34).
Change subject: sc7180: Add display hardware pipe line initialization ......................................................................
sc7180: Add display hardware pipe line initialization
Add sc7180 display hardware pipeline programming support and invoke the display initialization from soc_init.
Changes in V1: - added display init required check. - added edid read function using i2c communication. - added sn65dsi86 bridge driver to init bridge. - moved display initialization to mainboard file.
Changes in V2: - moved diplay init sequence to mainboard file - moved edid read function to bridge driver. - calculated timing paramters using edid parameters. - removed command mode config code. - moved bridge driver to drivers/ti. - seperated out bridge and soc code with mainboard file as interface.
Changes in V3: - add GPIO selection at runtime based on boardid. - add vbif register struct overlay.
Changes in V4: - update gpio config for lazor board.
Change-Id: I7d5e3f1781c48759553243abeb3d694f76cd008e Signed-off-by: Vinod Polimera vpolimer@codeaurora.org --- M src/mainboard/google/trogdor/board.h M src/mainboard/google/trogdor/mainboard.c M src/soc/qualcomm/sc7180/Kconfig M src/soc/qualcomm/sc7180/Makefile.inc A src/soc/qualcomm/sc7180/display/mdss.c 5 files changed, 263 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/39615/34
Vinod 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 34:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... File src/mainboard/google/trogdor/Kconfig:
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, Line 21: select DRIVERS_TI_SN65DSI86BRIDGE
Well, I mean it's still not fixed. […]
I have moved to correct order in the bridge patch, as the other patch required this change to fix build failure. So This is Done
https://review.coreboot.org/c/coreboot/+/39615/32/src/mainboard/google/trogd... File src/mainboard/google/trogdor/mainboard.c:
https://review.coreboot.org/c/coreboot/+/39615/32/src/mainboard/google/trogd... PS32, Line 8: #include "board.h"
Please order the headers with "double quotes" below all the headers with <angled brackets> with a bl […]
done
https://review.coreboot.org/c/coreboot/+/39615/32/src/mainboard/google/trogd... PS32, Line 93: 200
Update: Pompom guys confirmed that 250ms is enough to fix their issues, so please just update this n […]
done
https://review.coreboot.org/c/coreboot/+/39615/25/src/soc/qualcomm/sc7180/di... File src/soc/qualcomm/sc7180/display/mdss.c:
https://review.coreboot.org/c/coreboot/+/39615/25/src/soc/qualcomm/sc7180/di... PS25, Line 156: 0x00000003
*ping* (still unresolved)
Updated Comment. These are LUT values from HW settings.
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 Set 34: Code-Review+2
(2 comments)
LGTM but this patch order doesn't work, you have to reorder them!
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... File src/mainboard/google/trogdor/Kconfig:
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, Line 21: select DRIVERS_TI_SN65DSI86BRIDGE
I have moved to correct order in the bridge patch, as the other patch required this change to fix bu […]
Sorry, I don't understand what you mean. If you click on this patch https://review.coreboot.org/c/coreboot/+/42899/13 and you look at the "Relation chain" on the right, you will see that "sc7180: Add display hardware pipe line initialization" comes before "sc7180: Add support for sn65dsi86 bridge" in the list. However, "sc7180: Add display hardware pipe line initialization" calls the sn65dsi86_bridge_init() function which is implemented in "sc7180: Add support for sn65dsi86 bridge". You cannot call a function before it is implemented. Please just flip the order of the two patches around to resolve this build issue, the system won't let me merge it otherwise!
https://review.coreboot.org/c/coreboot/+/39615/32/src/mainboard/google/trogd... File src/mainboard/google/trogdor/mainboard.c:
https://review.coreboot.org/c/coreboot/+/39615/32/src/mainboard/google/trogd... PS32, Line 8: #include "board.h"
done
Ack
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 Set 35: -Code-Review
(1 comment)
https://review.coreboot.org/c/coreboot/+/39615/35/src/mainboard/google/trogd... File src/mainboard/google/trogdor/board.h:
https://review.coreboot.org/c/coreboot/+/39615/35/src/mainboard/google/trogd... PS35, Line 13: #define GPIO_WP_STATE GPIO(42) Just noticed one more thing, I'm pretty sure these two lines don't belong here?
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Julius Werner, mturney mturney, mturney mturney,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39615
to look at the new patch set (#36).
Change subject: sc7180: Add display hardware pipe line initialization ......................................................................
sc7180: Add display hardware pipe line initialization
Add sc7180 display hardware pipeline programming support and invoke the display initialization from soc_init.
Changes in V1: - added display init required check. - added edid read function using i2c communication. - added sn65dsi86 bridge driver to init bridge. - moved display initialization to mainboard file.
Changes in V2: - moved diplay init sequence to mainboard file - moved edid read function to bridge driver. - calculated timing paramters using edid parameters. - removed command mode config code. - moved bridge driver to drivers/ti. - seperated out bridge and soc code with mainboard file as interface.
Changes in V3: - add GPIO selection at runtime based on boardid. - add vbif register struct overlay.
Changes in V4: - update gpio config for lazor board.
Change-Id: I7d5e3f1781c48759553243abeb3d694f76cd008e Signed-off-by: Vinod Polimera vpolimer@codeaurora.org --- M src/mainboard/google/trogdor/board.h M src/mainboard/google/trogdor/mainboard.c M src/soc/qualcomm/sc7180/Kconfig M src/soc/qualcomm/sc7180/Makefile.inc A src/soc/qualcomm/sc7180/display/mdss.c 5 files changed, 264 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/39615/36
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Julius Werner, mturney mturney, mturney mturney,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39615
to look at the new patch set (#37).
Change subject: sc7180: Add display hardware pipe line initialization ......................................................................
sc7180: Add display hardware pipe line initialization
Add sc7180 display hardware pipeline programming support and invoke the display initialization from soc_init.
Changes in V1: - added display init required check. - added edid read function using i2c communication. - added sn65dsi86 bridge driver to init bridge. - moved display initialization to mainboard file.
Changes in V2: - moved diplay init sequence to mainboard file - moved edid read function to bridge driver. - calculated timing paramters using edid parameters. - removed command mode config code. - moved bridge driver to drivers/ti. - seperated out bridge and soc code with mainboard file as interface.
Changes in V3: - add GPIO selection at runtime based on boardid. - add vbif register struct overlay.
Changes in V4: - update gpio config for lazor board.
Change-Id: I7d5e3f1781c48759553243abeb3d694f76cd008e Signed-off-by: Vinod Polimera vpolimer@codeaurora.org --- M src/mainboard/google/trogdor/board.h M src/mainboard/google/trogdor/mainboard.c M src/soc/qualcomm/sc7180/Kconfig M src/soc/qualcomm/sc7180/Makefile.inc A src/soc/qualcomm/sc7180/display/mdss.c 5 files changed, 263 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/39615/37
Vinod 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 37:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39615/35/src/mainboard/google/trogd... File src/mainboard/google/trogdor/board.h:
https://review.coreboot.org/c/coreboot/+/39615/35/src/mainboard/google/trogd... PS35, Line 13: #define GPIO_WP_STATE GPIO(42)
Just noticed one more thing, I'm pretty sure these two lines don't belong here?
I am not sure where these gpios are used and owned by which subsystem.May be the right owner can address it ?
Vinod 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 37:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39615/35/src/mainboard/google/trogd... File src/mainboard/google/trogdor/board.h:
https://review.coreboot.org/c/coreboot/+/39615/35/src/mainboard/google/trogd... PS35, Line 13: #define GPIO_WP_STATE GPIO(42)
I am not sure where these gpios are used and owned by which subsystem. […]
Sorry, my bad. I will remove these lines.
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 Set 37:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39615/35/src/mainboard/google/trogd... File src/mainboard/google/trogdor/board.h:
https://review.coreboot.org/c/coreboot/+/39615/35/src/mainboard/google/trogd... PS35, Line 13: #define GPIO_WP_STATE GPIO(42)
Sorry, my bad. I will remove these lines.
Yes, they're unused, please just remove them.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Julius Werner, mturney mturney, mturney mturney,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39615
to look at the new patch set (#38).
Change subject: sc7180: Add display hardware pipe line initialization ......................................................................
sc7180: Add display hardware pipe line initialization
Add sc7180 display hardware pipeline programming support and invoke the display initialization from soc_init.
Changes in V1: - added display init required check. - added edid read function using i2c communication. - added sn65dsi86 bridge driver to init bridge. - moved display initialization to mainboard file.
Changes in V2: - moved diplay init sequence to mainboard file - moved edid read function to bridge driver. - calculated timing paramters using edid parameters. - removed command mode config code. - moved bridge driver to drivers/ti. - seperated out bridge and soc code with mainboard file as interface.
Changes in V3: - add GPIO selection at runtime based on boardid. - add vbif register struct overlay.
Changes in V4: - update gpio config for lazor board.
Change-Id: I7d5e3f1781c48759553243abeb3d694f76cd008e Signed-off-by: Vinod Polimera vpolimer@codeaurora.org --- M src/mainboard/google/trogdor/board.h M src/mainboard/google/trogdor/mainboard.c M src/soc/qualcomm/sc7180/Kconfig M src/soc/qualcomm/sc7180/Makefile.inc A src/soc/qualcomm/sc7180/display/mdss.c 5 files changed, 262 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/39615/38
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 Set 40: Code-Review+2
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 Set 40:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... File src/mainboard/google/trogdor/Kconfig:
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, Line 21: select DRIVERS_TI_SN65DSI86BRIDGE
Sorry, I don't understand what you mean. If you click on this patch https://review.coreboot. […]
Done
Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39615 )
Change subject: sc7180: Add display hardware pipe line initialization ......................................................................
sc7180: Add display hardware pipe line initialization
Add sc7180 display hardware pipeline programming support and invoke the display initialization from soc_init.
Changes in V1: - added display init required check. - added edid read function using i2c communication. - added sn65dsi86 bridge driver to init bridge. - moved display initialization to mainboard file.
Changes in V2: - moved diplay init sequence to mainboard file - moved edid read function to bridge driver. - calculated timing paramters using edid parameters. - removed command mode config code. - moved bridge driver to drivers/ti. - seperated out bridge and soc code with mainboard file as interface.
Changes in V3: - add GPIO selection at runtime based on boardid. - add vbif register struct overlay.
Changes in V4: - update gpio config for lazor board.
Change-Id: I7d5e3f1781c48759553243abeb3d694f76cd008e Signed-off-by: Vinod Polimera vpolimer@codeaurora.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/39615 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Julius Werner jwerner@chromium.org --- M src/mainboard/google/trogdor/board.h M src/mainboard/google/trogdor/mainboard.c M src/soc/qualcomm/sc7180/Kconfig M src/soc/qualcomm/sc7180/Makefile.inc A src/soc/qualcomm/sc7180/display/mdss.c 5 files changed, 262 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved
diff --git a/src/mainboard/google/trogdor/board.h b/src/mainboard/google/trogdor/board.h index bd7222f..39661b5 100644 --- a/src/mainboard/google/trogdor/board.h +++ b/src/mainboard/google/trogdor/board.h @@ -13,6 +13,11 @@ #define GPIO_SD_CD_L GPIO(69) #define GPIO_AMP_ENABLE GPIO(23)
+/* Display specific GPIOS */ +#define GPIO_BACKLIGHT_ENABLE GPIO(12) +#define GPIO_EDP_BRIDGE_ENABLE (CONFIG(TROGDOR_REV0) ? GPIO(14) : GPIO(104)) +#define GPIO_EN_PP3300_DX_EDP (CONFIG(TROGDOR_REV0) ? GPIO(106) : GPIO(30)) + void setup_chromeos_gpios(void);
#endif /* _COREBOOT_SRC_MAINBOARD_GOOGLE_TROGDOR_BOARD_H_ */ diff --git a/src/mainboard/google/trogdor/mainboard.c b/src/mainboard/google/trogdor/mainboard.c index 349c306..57f3a3b 100644 --- a/src/mainboard/google/trogdor/mainboard.c +++ b/src/mainboard/google/trogdor/mainboard.c @@ -1,9 +1,21 @@ /* SPDX-License-Identifier: GPL-2.0-only */
+#include <bootmode.h> +#include <delay.h> #include <device/device.h> +#include <device/i2c_simple.h> +#include <drivers/ti/sn65dsi86bridge/sn65dsi86bridge.h> +#include <soc/display/mipi_dsi.h> +#include <soc/display/mdssreg.h> #include <soc/qupv3_config.h> +#include <soc/qupv3_i2c.h> #include <soc/usb.h>
+#include "board.h" + +#define BRIDGE_BUS 0x2 +#define BRIDGE_CHIP 0x2d + static struct usb_board_data usb0_board_data = { .pll_bias_control_2 = 0x22, .imp_ctrl1 = 0x08, @@ -20,7 +32,6 @@
static void qi2s_configure_gpios(void) { - gpio_configure(GPIO(49), GPIO49_FUNC_MI2S_1_SCK, GPIO_NO_PULL, GPIO_8MA, GPIO_OUTPUT);
@@ -34,7 +45,6 @@ static void load_qup_fw(void) { qupv3_se_fw_load_and_init(QUPV3_0_SE1, SE_PROTOCOL_SPI, MIXED); /* ESIM SPI */ - qupv3_se_fw_load_and_init(QUPV3_0_SE2, SE_PROTOCOL_I2C, MIXED); /* EDP Bridge I2C */ qupv3_se_fw_load_and_init(QUPV3_0_SE3, SE_PROTOCOL_UART, FIFO); /* BT UART */ qupv3_se_fw_load_and_init(QUPV3_0_SE4, SE_PROTOCOL_I2C, MIXED); /* Pen Detect I2C */ qupv3_se_fw_load_and_init(QUPV3_0_SE5, SE_PROTOCOL_I2C, MIXED); /* SAR I2C */ @@ -51,11 +61,60 @@ qupv3_se_fw_load_and_init(QUPV3_1_SE5, SE_PROTOCOL_I2C, MIXED); /* Codec I2C */ }
+static void configure_display(void) +{ + printk(BIOS_INFO, "%s: Bridge gpio init\n", __func__); + + /* Bridge Enable GPIO */ + gpio_output(GPIO_EDP_BRIDGE_ENABLE, 1); + + /* PP3300 EDP power supply */ + gpio_output(GPIO_EN_PP3300_DX_EDP, 1); +} + +static void display_init(struct edid *edid) +{ + uint32_t dsi_bpp = 24; + uint32_t lanes = 4; + + if (mdss_dsi_config(edid, lanes, dsi_bpp)) + return; + + sn65dsi86_bridge_configure(BRIDGE_BUS, BRIDGE_CHIP, edid, lanes, dsi_bpp); + mdp_dsi_video_config(edid); + mdss_dsi_video_mode_config(edid, dsi_bpp); + mdp_dsi_video_on(); +} + +static void display_startup(void) +{ + static struct edid ed; + enum dp_pll_clk_src ref_clk = SN65_SEL_19MHZ; + + i2c_init(QUPV3_0_SE2, I2C_SPEED_FAST); /* EDP Bridge I2C */ + if (display_init_required()) { + configure_display(); + mdelay(250); /* Delay for the panel to be up */ + sn65dsi86_bridge_init(BRIDGE_BUS, BRIDGE_CHIP, ref_clk); + if (sn65dsi86_bridge_read_edid(BRIDGE_BUS, BRIDGE_CHIP, &ed) < 0) + return; + + printk(BIOS_INFO, "display init!\n"); + + /* Configure backlight */ + gpio_output(GPIO_BACKLIGHT_ENABLE, 1); + display_init(&ed); + set_vbe_mode_info_valid(&ed, (uintptr_t)0); + } else + printk(BIOS_INFO, "Skipping display init.\n"); +} + static void mainboard_init(struct device *dev) { setup_usb(); qi2s_configure_gpios(); load_qup_fw(); + display_startup(); }
static void mainboard_enable(struct device *dev) diff --git a/src/soc/qualcomm/sc7180/Kconfig b/src/soc/qualcomm/sc7180/Kconfig index 17df0d0..d543ef5 100644 --- a/src/soc/qualcomm/sc7180/Kconfig +++ b/src/soc/qualcomm/sc7180/Kconfig @@ -13,6 +13,10 @@ select ARM64_USE_ARCH_TIMER select SOC_QUALCOMM_COMMON select HAVE_UART_SPECIAL + select BOOTBLOCK_CONSOLE + select MAINBOARD_HAS_NATIVE_VGA_INIT + select MAINBOARD_FORCE_NATIVE_VGA_INIT + select HAVE_LINEAR_FRAMEBUFFER
if SOC_QUALCOMM_SC7180
diff --git a/src/soc/qualcomm/sc7180/Makefile.inc b/src/soc/qualcomm/sc7180/Makefile.inc index ec6ab6c..eea38d9 100644 --- a/src/soc/qualcomm/sc7180/Makefile.inc +++ b/src/soc/qualcomm/sc7180/Makefile.inc @@ -62,6 +62,7 @@ ramstage-$(CONFIG_MAINBOARD_DO_NATIVE_VGA_INIT) += display/dsi_phy_pll.c ramstage-$(CONFIG_MAINBOARD_DO_NATIVE_VGA_INIT) += display/dsi_phy.c ramstage-$(CONFIG_MAINBOARD_DO_NATIVE_VGA_INIT) += display/dsi.c +ramstage-$(CONFIG_MAINBOARD_DO_NATIVE_VGA_INIT) += display/mdss.c
################################################################################
diff --git a/src/soc/qualcomm/sc7180/display/mdss.c b/src/soc/qualcomm/sc7180/display/mdss.c new file mode 100644 index 0000000..2d6bf6f --- /dev/null +++ b/src/soc/qualcomm/sc7180/display/mdss.c @@ -0,0 +1,191 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <device/mmio.h> +#include <console/console.h> +#include <delay.h> +#include <edid.h> +#include <soc/display/mdssreg.h> + +#define MDSS_MDP_MAX_PREFILL_FETCH 25 + +static void mdss_source_pipe_config(struct edid *edid) +{ + uint32_t img_size, out_size, stride; + uint32_t fb_off = 0; + uint32_t flip_bits = 0; + uint32_t src_xy = 0; + uint32_t dst_xy = 0; + + /* write active region size*/ + img_size = (edid->mode.va << 16) | edid->mode.ha; + out_size = img_size; + stride = (edid->mode.ha * edid->framebuffer_bits_per_pixel/8); + + if (!fb_off) { /* left */ + dst_xy = (edid->mode.vborder << 16) | edid->mode.hborder; + src_xy = dst_xy; + } else { /* right */ + dst_xy = (edid->mode.vborder << 16); + src_xy = (edid->mode.vborder << 16) | fb_off; + } + + printk(BIOS_INFO, "%s: src=%x fb_off=%x src_xy=%x dst_xy=%x\n", + __func__, out_size, fb_off, src_xy, dst_xy); + + write32(&mdp_sspp->sspp_src_ystride0, stride); + write32(&mdp_sspp->sspp_src_size, out_size); + write32(&mdp_sspp->sspp_out_size, out_size); + write32(&mdp_sspp->sspp_src_xy, src_xy); + write32(&mdp_sspp->sspp_out_xy, dst_xy); + + /* Tight Packing 4bpp Alpha 8-bit A R B G */ + write32(&mdp_sspp->sspp_src_format, 0x000236ff); + write32(&mdp_sspp->sspp_src_unpack_pattern, 0x03020001); + + flip_bits |= BIT(31); + write32(&mdp_sspp->sspp_sw_pic_ext_c0_req_pixels, out_size); + write32(&mdp_sspp->sspp_sw_pic_ext_c1c2_req_pixels, out_size); + write32(&mdp_sspp->sspp_sw_pic_ext_c3_req_pixels, out_size); + write32(&mdp_sspp->sspp_src_op_mode, flip_bits); +} + +static void mdss_vbif_setup(void) +{ + write32(&vbif_rt->vbif_out_axi_amemtype_conf0, 0x33333333); + write32(&vbif_rt->vbif_out_axi_amemtype_conf1, 0x00333333); +} + +static void mdss_intf_tg_setup(struct edid *edid) +{ + uint32_t hsync_period, vsync_period; + uint32_t hsync_start_x, hsync_end_x; + uint32_t display_hctl, hsync_ctl, display_vstart, display_vend; + + hsync_period = edid->mode.ha + edid->mode.hbl; + vsync_period = edid->mode.va + edid->mode.vbl; + hsync_start_x = edid->mode.hbl - edid->mode.hso; + hsync_end_x = hsync_period - edid->mode.hso - 1; + display_vstart = (edid->mode.vbl - edid->mode.vso) * hsync_period; + display_vend = ((vsync_period - edid->mode.vso) * hsync_period) - 1; + hsync_ctl = (hsync_period << 16) | edid->mode.hspw; + display_hctl = (hsync_end_x << 16) | hsync_start_x; + + write32(&mdp_intf->intf_hsync_ctl, hsync_ctl); + write32(&mdp_intf->intf_vysnc_period_f0, + vsync_period * hsync_period); + write32(&mdp_intf->intf_vysnc_pulse_width_f0, + edid->mode.vspw * hsync_period); + write32(&mdp_intf->intf_disp_hctl, display_hctl); + write32(&mdp_intf->intf_disp_v_start_f0, display_vstart); + write32(&mdp_intf->intf_disp_v_end_f0, display_vend); + write32(&mdp_intf->intf_underflow_color, 0x00); + write32(&mdp_intf->intf_panel_format, 0x2100); +} + +static void mdss_intf_fetch_start_config(struct edid *edid) +{ + uint32_t v_total, h_total, fetch_start, vfp_start; + uint32_t prefetch_avail, prefetch_needed; + uint32_t fetch_enable = BIT(31); + + /* + * MDP programmable fetch is for MDP with rev >= 1.05. + * Programmable fetch is not needed if vertical back porch + * plus vertical puls width is >= 25. + */ + if ((edid->mode.vbl - edid->mode.vso) >= MDSS_MDP_MAX_PREFILL_FETCH) + return; + + /* + * Fetch should always be outside the active lines. If the fetching + * is programmed within active region, hardware behavior is unknown. + */ + v_total = edid->mode.va + edid->mode.vbl; + h_total = edid->mode.ha + edid->mode.hbl; + vfp_start = edid->mode.va + edid->mode.vbl - edid->mode.vso; + prefetch_avail = v_total - vfp_start; + prefetch_needed = MDSS_MDP_MAX_PREFILL_FETCH - edid->mode.vbl + edid->mode.vso; + + /* + * In some cases, vertical front porch is too high. In such cases limit + * the mdp fetch lines as the last (25 - vbp - vpw) lines of + * vertical front porch. + */ + if (prefetch_avail > prefetch_needed) + prefetch_avail = prefetch_needed; + + fetch_start = (v_total - prefetch_avail) * h_total + h_total + 1; + write32(&mdp_intf->intf_prof_fetch_start, fetch_start); + write32(&mdp_intf->intf_config, fetch_enable); +} + +static void mdss_layer_mixer_setup(struct edid *edid) +{ + uint32_t mdp_rgb_size; + uint32_t left_staging_level; + + /* write active region size*/ + mdp_rgb_size = (edid->mode.va << 16) | edid->mode.ha; + + write32(&mdp_layer_mixer->layer_out_size, mdp_rgb_size); + write32(&mdp_layer_mixer->layer_op_mode, 0x0); + for (int i = 0; i < 6; i++) { + write32(&mdp_layer_mixer->layer_blend[i].layer_blend_op, 0x100); + write32(&mdp_layer_mixer->layer_blend[i].layer_blend_const_alpha, 0x00ff0000); + } + + /* Enable border fill */ + left_staging_level = BIT(24); + left_staging_level |= BIT(1); + + /* Base layer for layer mixer 0 */ + write32(&mdp_ctl->ctl_layer0, left_staging_level); +} + +static void mdss_vbif_qos_remapper_setup(void) +{ + /* + * VBIF remapper registers are used for translating internal display hardware + * priority level (from 0 to 7) into system fabric priority level. + * These remapper settings are defined for all the clients which corresponds + * to the xin clients connected to SSPP on VBIF. + */ + write32(&vbif_rt->qos_rp_remap[0].vbif_xinl_qos_rp_remap, 0x00000003); + write32(&vbif_rt->qos_rp_remap[1].vbif_xinl_qos_rp_remap, 0x11111113); + write32(&vbif_rt->qos_rp_remap[2].vbif_xinl_qos_rp_remap, 0x22222224); + write32(&vbif_rt->qos_rp_remap[3].vbif_xinl_qos_rp_remap, 0x33333334); + write32(&vbif_rt->qos_rp_remap[4].vbif_xinl_qos_rp_remap, 0x44444445); + write32(&vbif_rt->qos_rp_remap[7].vbif_xinl_qos_rp_remap, 0x77777776); + write32(&vbif_rt->qos_lvl_remap[0].vbif_xinl_qos_lvl_remap, 0x00000003); + write32(&vbif_rt->qos_lvl_remap[1].vbif_xinl_qos_lvl_remap, 0x11111113); + write32(&vbif_rt->qos_lvl_remap[2].vbif_xinl_qos_lvl_remap, 0x22222224); + write32(&vbif_rt->qos_lvl_remap[3].vbif_xinl_qos_lvl_remap, 0x33333334); + write32(&vbif_rt->qos_lvl_remap[4].vbif_xinl_qos_lvl_remap, 0x44444445); + write32(&vbif_rt->qos_lvl_remap[5].vbif_xinl_qos_lvl_remap, 0x77777776); +} + +void mdp_dsi_video_config(struct edid *edid) +{ + mdss_intf_tg_setup(edid); + mdss_intf_fetch_start_config(edid); + mdss_vbif_setup(); + mdss_vbif_qos_remapper_setup(); + mdss_source_pipe_config(edid); + mdss_layer_mixer_setup(edid); + + /* Select Video Mode Interface */ + write32(&mdp_ctl->ctl_top, 0x0); + + /* PPB0 to INTF1 */ + write32(&mdp_ctl->ctl_intf_active, BIT(1)); + write32(&mdp_intf->intf_mux, 0x0F0000); +} + +void mdp_dsi_video_on(void) +{ + uint32_t ctl0_reg_val; + + ctl0_reg_val = VIG_0 | LAYER_MIXER_0 | CTL | INTF; + write32(&mdp_ctl->ctl_intf_flush, 0x2); + write32(&mdp_ctl->ctl_flush, ctl0_reg_val); +}
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39615 )
Change subject: sc7180: Add display hardware pipe line initialization ......................................................................
Patch Set 41:
Automatic boot test returned (PASS/FAIL/TOTAL): 7/1/8 "QEMU x86 q35/ich9" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/18587 "QEMU x86 q35/ich9" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/18586 "QEMU x86 i440fx/piix4" (x86_64) using payload SeaBIOS : FAIL : https://lava.9esec.io/r/18585 "QEMU x86 i440fx/piix4" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/18584 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/18583 "HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/18590 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/18589 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/18588
Please note: This test is under development and might not be accurate at all!