Ravi kumar has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42899 )
Change subject: sc7180: Add support for sn65dsi86 bridge. ......................................................................
sc7180: Add support for sn65dsi86 bridge.
Add sn65dsi86 bridge driver to enable the eDP bridge.
Change-Id: I36a68f3241f0ba316c261a73c2f6d30fe6c3ccdc Signed-off-by: Vinod Polimera vpolimer@codeaurora.org --- A src/drivers/ti/sn65dsi86bridge/Kconfig A src/drivers/ti/sn65dsi86bridge/Makefile.inc A src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c A src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.h 4 files changed, 548 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/42899/1
diff --git a/src/drivers/ti/sn65dsi86bridge/Kconfig b/src/drivers/ti/sn65dsi86bridge/Kconfig new file mode 100644 index 0000000..1a37409 --- /dev/null +++ b/src/drivers/ti/sn65dsi86bridge/Kconfig @@ -0,0 +1,7 @@ +## SPDX-License-Identifier: GPL-2.0-only + +config DRIVERS_TI_SN65DSI86BRIDGE + bool + default y + help + TI SN65DSI86BRIDGE diff --git a/src/drivers/ti/sn65dsi86bridge/Makefile.inc b/src/drivers/ti/sn65dsi86bridge/Makefile.inc new file mode 100644 index 0000000..c46eb6d --- /dev/null +++ b/src/drivers/ti/sn65dsi86bridge/Makefile.inc @@ -0,0 +1,3 @@ +## SPDX-License-Identifier: GPL-2.0-only + +ramstage-$(CONFIG_DRIVERS_TI_SN65DSI86BRIDGE) += sn65dsi86bridge.c \ No newline at end of file diff --git a/src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c b/src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c new file mode 100644 index 0000000..3fda4f9 --- /dev/null +++ b/src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c @@ -0,0 +1,398 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <console/console.h> +#include <delay.h> +#include <endian.h> +#include <device/i2c_simple.h> +#include <edid.h> +#include <timer.h> +#include <types.h> +#include <soc/addressmap.h> +#include "sn65dsi86bridge.h" + +#define bridge_debug(x...) do {if (0) printk(BIOS_DEBUG, x); } while (0) + +#define BRIDGE_GETHIGHERBYTE(x) (uint8_t)((x&0xFF00)>>8) +#define BRIDGE_GETLOWERBYTE(x) (uint8_t)((x&0x00FF)) + +/* fudge factor required to account for 8b/10b encoding */ +#define DP_CLK_FUDGE_NUM 10 +#define DP_CLK_FUDGE_DEN 8 + +/* DPCD */ +#define DP_BRIDGE_DPCD_REV 0x700 +#define DP_BRIDGE_11 0x00 +#define DP_BRIDGE_12 0x01 +#define DP_BRIDGE_13 0x02 +#define DP_BRIDGE_14 0x03 +#define DP_BRIDGE_CONFIGURATION_SET 0x10a +#define DP_MAX_LINK_RATE 0x001 +#define DP_MAX_LANE_COUNT 0x002 +#define DP_SUPPORTED_LINK_RATES 0x010 /* eDP 1.4 */ +#define DP_MAX_LINK_RATE 0x001 +#define DP_MAX_SUPPORTED_RATES 8 /* 16-bit little-endian */ + +/* link configuration */ +#define DP_LINK_BW_SET 0x100 +#define DP_LINK_BW_1_62 0x06 +#define DP_LINK_BW_2_7 0x0a +#define DP_LINK_BW_5_4 0x14 + +#define AUX_CMD_SEND 0x1 +#define MIN_DSI_CLK_FREQ_MHZ 40 + +/* + * LUT index corresponds to register value and + * LUT values corresponds to dp data rate supported + * by the bridge in Mbps unit. + */ +static const unsigned int sn65dsi86_bridge_dp_rate_lut[] = { + 0, 1620, 2160, 2430, 2700, 3240, 4320, 5400 +}; + +enum cb_err sn65dsi86_bridge_read_edid(uint8_t bus, uint8_t chip, struct edid *out) +{ + int ret; + u8 edid[EDID_LENGTH * 2]; + int edid_size = EDID_LENGTH; + + /* Send I2C command to Disable the HPD */ + i2c_writeb(bus, chip, SN_HPD_DISABLE_REG, 0x1); + + /* Send I2C command to claim EDID I2c slave */ + i2c_writeb(bus, chip, I2C_CLAIM_ADDR_EN1, (EDID_I2C_ADDR << 1) | 0x1); + + /* read EDID */ + ret = i2c_read_bytes(bus, EDID_I2C_ADDR, 0x0, edid, EDID_LENGTH); + if (ret != 0) { + printk(BIOS_ERR, "ERROR: Failed to read EDID.\n"); + return CB_ERR; + } + + if (edid[EDID_EXTENSION_FLAG]) { + edid_size += EDID_LENGTH; + ret = i2c_read_bytes(bus, EDID_I2C_ADDR, EDID_LENGTH, + &edid[EDID_LENGTH], EDID_LENGTH); + if (ret != 0) { + printk(BIOS_ERR, "Failed to read EDID ext block.\n"); + return CB_ERR; + } + } + + if (decode_edid(edid, edid_size, out) != EDID_CONFORMANT) { + printk(BIOS_ERR, "ERROR: Failed to decode EDID.\n"); + return CB_ERR; + } + + return CB_SUCCESS; +} + +static void sn65dsi86_bridge_dpcd_request(uint8_t bus, + uint8_t chip, + unsigned int dpcd_reg, + unsigned int len, + enum dpcd_request request, + uint8_t *data) +{ + int i; + uint32_t length; + uint8_t buf; + uint8_t reg; + + while (len) { + length = MIN(len, 16); + + i2c_writeb(bus, chip, SN_AUX_ADDR_19_16_REG, (dpcd_reg >> 16) & 0xF); + i2c_writeb(bus, chip, SN_AUX_ADDR_15_8_REG, (dpcd_reg >> 8) & 0xFF); + i2c_writeb(bus, chip, SN_AUX_ADDR_7_0_REG, (dpcd_reg) & 0xFF); + i2c_writeb(bus, chip, SN_AUX_LENGTH_REG, length); /* size of 1 Byte data */ + if (request == DPCD_WRITE) { + reg = SN_AUX_WDATA_REG_0; + for (i = 0; i < length; i++) + i2c_writeb(bus, chip, reg++, *data++); + + i2c_writeb(bus, chip, + SN_AUX_CMD_REG, AUX_CMD_SEND | (NATIVE_AUX_WRITE << 4)); + } else { + i2c_writeb(bus, chip, + SN_AUX_CMD_REG, AUX_CMD_SEND | (NATIVE_AUX_READ << 4)); + if (!wait_ms(100, + !i2c_readb(bus, chip, SN_AUX_CMD_REG, + &buf) && !(buf & AUX_CMD_SEND))) { + printk(BIOS_ERR, "ERROR: aux command send failed\n"); + } + + reg = SN_AUX_RDATA_REG_0; + for (i = 0; i < length; i++) { + i2c_readb(bus, chip, reg++, &buf); + *data++ = buf; + } + } + + len -= length; + } +} + +static void sn65dsi86_bridge_valid_dp_rates(uint8_t bus, uint8_t chip, bool rate_valid[]) +{ + unsigned int rate_per_200khz; + unsigned int rate_mhz; + uint8_t dpcd_val; + int i, j; + + sn65dsi86_bridge_dpcd_request(bus, chip, + DP_BRIDGE_DPCD_REV, 1, DPCD_READ, &dpcd_val); + if (dpcd_val >= DP_BRIDGE_14) { + /* eDP 1.4 devices must provide a custom table */ + uint8_t sink_rates[DP_MAX_SUPPORTED_RATES * 2]; + + sn65dsi86_bridge_dpcd_request(bus, chip, DP_SUPPORTED_LINK_RATES, + ARRAY_SIZE(sink_rates), + DPCD_READ, sink_rates); + for (i = 0; i < ARRAY_SIZE(sink_rates); i++) { + rate_per_200khz = le16_to_cpu(sink_rates[i]); + + if (!rate_per_200khz) + break; + + rate_mhz = rate_per_200khz * 200 / 1000; + for (j = 0; + j < ARRAY_SIZE(sn65dsi86_bridge_dp_rate_lut); + j++) { + if (sn65dsi86_bridge_dp_rate_lut[j] == rate_mhz) + rate_valid[j] = true; + } + } + + for (i = 0; i < ARRAY_SIZE(sn65dsi86_bridge_dp_rate_lut); i++) { + if (rate_valid[i]) + return; + } + + printk(BIOS_ERR, "No matching eDP rates in table; falling back\n"); + } + + /* On older versions best we can do is use DP_MAX_LINK_RATE */ + sn65dsi86_bridge_dpcd_request(bus, chip, + DP_MAX_LINK_RATE, 1, DPCD_READ, &dpcd_val); + + switch (dpcd_val) { + default: + printk(BIOS_ERR, + "Unexpected max rate (%#x); assuming 5.4 GHz\n", + (int)dpcd_val); + /* fall through */ + case DP_LINK_BW_5_4: + rate_valid[7] = 1; + /* fall through */ + case DP_LINK_BW_2_7: + rate_valid[4] = 1; + /* fall through */ + case DP_LINK_BW_1_62: + rate_valid[1] = 1; + break; + } +} + +static void sn65dsi86_bridge_set_dsi_clock_range(uint8_t bus, uint8_t chip, + struct edid *edid, + int num_of_lanes, int bpp) +{ + uint64_t pixel_clk_hz; + uint64_t stream_bit_rate_mhz; + uint64_t min_req_dsi_clk; + + pixel_clk_hz = edid->mode.pixel_clock * 1000; + stream_bit_rate_mhz = (pixel_clk_hz / 1000000) * bpp; + + /* For TI the clock frequencies are half the bit rates */ + min_req_dsi_clk = stream_bit_rate_mhz / (num_of_lanes * 2); + + /* for each increment in val, frequency increases by 5MHz */ + min_req_dsi_clk = (MIN_DSI_CLK_FREQ_MHZ / 5) + + (((min_req_dsi_clk - MIN_DSI_CLK_FREQ_MHZ) / 5) & 0xFF); + + i2c_writeb(bus, chip, SN_DSIA_CLK_FREQ_REG, min_req_dsi_clk); +} + +static void sn65dsi86_bridge_set_dp_clock_range(uint8_t bus, uint8_t chip, + struct edid *edid, int num_of_lanes) +{ + uint64_t stream_bit_rate_khz; + bool rate_valid[ARRAY_SIZE(sn65dsi86_bridge_dp_rate_lut)] = { }; + uint64_t dp_rate_mhz; + int dp_rate_idx, i; + + stream_bit_rate_khz = edid->mode.pixel_clock * 18; + + /* Calculate minimum DP data rate, taking 80% as per DP spec */ + dp_rate_mhz = DIV_ROUND_UP(stream_bit_rate_khz * DP_CLK_FUDGE_NUM, + 1000 * num_of_lanes * DP_CLK_FUDGE_DEN); + + for (i = 0; i < ARRAY_SIZE(sn65dsi86_bridge_dp_rate_lut) - 1; i++) + if (sn65dsi86_bridge_dp_rate_lut[i] > dp_rate_mhz) + break; + + sn65dsi86_bridge_valid_dp_rates(bus, chip, rate_valid); + + /* Train until we run out of rates */ + for (dp_rate_idx = i; + dp_rate_idx < ARRAY_SIZE(sn65dsi86_bridge_dp_rate_lut); + dp_rate_idx++) + if (rate_valid[dp_rate_idx]) + break; + + if(dp_rate_idx < ARRAY_SIZE(sn65dsi86_bridge_dp_rate_lut)) + i2c_write_field(bus, chip, SN_DATARATE_CONFIG_REG, dp_rate_idx, 8, 5); + else + printk(BIOS_ERR, "ERROR: valid dp rate not found"); +} + +static void sn65dsi86_bridge_set_bridge_active_timing(uint8_t bus, + uint8_t chip, + struct edid *edid) +{ + i2c_writeb(bus, chip, SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG, + BRIDGE_GETLOWERBYTE(edid->mode.ha)); + i2c_writeb(bus, chip, SN_CHA_ACTIVE_LINE_LENGTH_HIGH_REG, + BRIDGE_GETHIGHERBYTE(edid->mode.ha)); + i2c_writeb(bus, chip, SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG, + BRIDGE_GETLOWERBYTE(edid->mode.va)); + i2c_writeb(bus, chip, SN_CHA_VERTICAL_DISPLAY_SIZE_HIGH_REG, + BRIDGE_GETHIGHERBYTE(edid->mode.va)); + i2c_writeb(bus, chip, SN_CHA_HSYNC_PULSE_WIDTH_LOW_REG, + BRIDGE_GETLOWERBYTE(edid->mode.hspw)); + i2c_writeb(bus, chip, SN_CHA_HSYNC_PULSE_WIDTH_HIGH_REG, + BRIDGE_GETHIGHERBYTE(edid->mode.hspw)); + i2c_writeb(bus, chip, SN_CHA_VSYNC_PULSE_WIDTH_LOW_REG, + BRIDGE_GETLOWERBYTE(edid->mode.vspw)); + i2c_writeb(bus, chip, SN_CHA_VSYNC_PULSE_WIDTH_HIGH_REG, + BRIDGE_GETHIGHERBYTE(edid->mode.vspw)); + i2c_writeb(bus, chip, SN_CHA_HORIZONTAL_BACK_PORCH_REG, + edid->mode.hbl - edid->mode.hso - edid->mode.hspw); + i2c_writeb(bus, chip, SN_CHA_VERTICAL_BACK_PORCH_REG, + edid->mode.vbl - edid->mode.vso - edid->mode.vspw); + i2c_writeb(bus, chip, SN_CHA_HORIZONTAL_FRONT_PORCH_REG, + edid->mode.hso); + i2c_writeb(bus, chip, SN_CHA_VERTICAL_FRONT_PORCH_REG, + edid->mode.vso); +} + +static void sn65dsi86_bridge_link_training(uint8_t bus, uint8_t chip) +{ + uint8_t buf; + + /* enable pll lock */ + i2c_writeb(bus, chip, SN_PLL_ENABLE_REG, 0x1); + + if (!wait_ms(500, + !(i2c_readb(bus, chip, SN_DPPLL_SRC_REG, &buf)) && + (buf & BIT(7)))) { + printk(BIOS_ERR, "ERROR: PLL lock failure\n"); + } + + /* + * The SN65DSI86 only supports ASSR Display Authentication method and + * this method is enabled by default. An eDP panel must support this + * authentication method. We need to enable this method in the eDP panel + * at DisplayPort address 0x0010A prior to link training. + */ + buf = 0x1; + sn65dsi86_bridge_dpcd_request(bus, chip, + DP_BRIDGE_CONFIGURATION_SET, 1, DPCD_WRITE, &buf); + + /* semi auto link training mode */ + i2c_writeb(bus, chip, SN_ML_TX_MODE_REG, 0xa); + + if (!wait_ms(500, + !(i2c_readb(bus, chip, SN_ML_TX_MODE_REG, &buf)) && + (buf & NORMAL_MODE))) { + printk(BIOS_ERR, "ERROR: Link training failed"); + } + +} + +static enum cb_err sn65dsi86_bridge_get_plug_in_status(uint8_t bus, uint8_t chip) +{ + int val; + uint8_t buf; + + val = i2c_readb(bus, chip, SN_HPD_DISABLE_REG, &buf); + if (val == 0 && (buf & HPD_DISABLE)) + return CB_SUCCESS; + + return CB_ERR; +} + +/* + * support bridge HPD function + * some hardware version do not support bridge hdp, + * we use 360ms to try to get the hpd single now, + * if we can not get bridge hpd single, it will delay 360ms, + * also meet the bridge power timing request, to compatible + * all of the hardware version + */ +static void sn65dsi86_bridge_wait_hpd(uint8_t bus, uint8_t chip) +{ + if (wait_ms(360, sn65dsi86_bridge_get_plug_in_status(bus, chip))) + return; + + printk(BIOS_WARNING, "HPD detection failed, force hpd\n"); + + /* Force HPD */ + i2c_write_field(bus, chip, SN_HPD_DISABLE_REG, HPD_DISABLE, 1, 0 ); +} + +static void sn65dsi86_bridge_assr_config(uint8_t bus, uint8_t chip, int enable) +{ + if (enable) + i2c_write_field(bus, chip, SN_ENH_FRAME_REG, VSTREAM_ENABLE, 1, 3); + else + i2c_write_field(bus, chip, SN_ENH_FRAME_REG, VSTREAM_DISABLE, 1, 3); +} + +static int sn65dsi86_bridge_dp_lane_config(uint8_t bus, uint8_t chip) +{ + uint8_t data; + + sn65dsi86_bridge_dpcd_request(bus, chip, DP_MAX_LANE_COUNT, 1, DPCD_READ, &data); + i2c_write_field(bus, chip, SN_SSC_CONFIG_REG, MIN(data, 3), 3, 4); + + return data; +} + +void sn65dsi86_bridge_init(uint8_t bus, uint8_t chip, + struct edid *edid, uint32_t num_of_lanes, + uint8_t ref_clk, uint32_t bpp) +{ + int dp_lanes; + + sn65dsi86_bridge_wait_hpd(bus, chip); + + /* set refclk to 19.2 MHZ */ + i2c_write_field(bus, chip, SN_DPPLL_SRC_REG, ref_clk, 7, 1); + + /* DSI Lanes config */ + i2c_write_field(bus, chip, SN_DSI_LANES_REG, (4 - num_of_lanes), 3, 3); + + /* DP Lane config */ + dp_lanes = sn65dsi86_bridge_dp_lane_config(bus, chip); + + sn65dsi86_bridge_set_dsi_clock_range(bus, chip, edid, num_of_lanes ,bpp); + + sn65dsi86_bridge_set_dp_clock_range(bus, chip, edid, dp_lanes); + + /* Disable vstream */ + sn65dsi86_bridge_assr_config(bus, chip, 0); + sn65dsi86_bridge_link_training(bus, chip); + sn65dsi86_bridge_set_bridge_active_timing(bus, chip, edid); + + /* DP BPP config */ + i2c_writeb(bus, chip, SN_DATA_FORMAT_REG, (uint8_t)0x01); + + /* color bar disabled */ + i2c_writeb(bus, chip, SN_COLOR_BAR_REG, 0x5); + + /* Enable vstream */ + sn65dsi86_bridge_assr_config(bus, chip, 1); +} diff --git a/src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.h b/src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.h new file mode 100644 index 0000000..b8a9b6e --- /dev/null +++ b/src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.h @@ -0,0 +1,140 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef __TI_SN65DSI86BRIDGE_H +#define __TI_SN65DSI86BRIDGE_H + +#include <edid.h> + +enum bridge_regs { + SN_DPPLL_SRC_REG = 0x0A, + SN_PLL_ENABLE_REG = 0x0D, + SN_DSI_LANES_REG = 0x10, + SN_DSIA_CLK_FREQ_REG = 0x12, + SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG = 0x20, + SN_CHA_ACTIVE_LINE_LENGTH_HIGH_REG = 0x21, + SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG = 0x24, + SN_CHA_VERTICAL_DISPLAY_SIZE_HIGH_REG = 0x25, + SN_CHA_HSYNC_PULSE_WIDTH_LOW_REG = 0x2C, + SN_CHA_HSYNC_PULSE_WIDTH_HIGH_REG = 0x2D, + SN_CHA_VSYNC_PULSE_WIDTH_LOW_REG = 0x30, + SN_CHA_VSYNC_PULSE_WIDTH_HIGH_REG = 0x31, + SN_CHA_HORIZONTAL_BACK_PORCH_REG = 0x34, + SN_CHA_VERTICAL_BACK_PORCH_REG = 0x36, + SN_CHA_HORIZONTAL_FRONT_PORCH_REG = 0x38, + SN_CHA_VERTICAL_FRONT_PORCH_REG = 0x3A, + SN_COLOR_BAR_REG = 0x3C, + SN_ENH_FRAME_REG = 0x5A, + SN_DATA_FORMAT_REG = 0x5B, + SN_HPD_DISABLE_REG = 0x5C, + SN_AUX_WDATA_REG_0 = 0x64, + SN_AUX_WDATA_REG_1 = 0x65, + SN_AUX_WDATA_REG_2 = 0x66, + SN_AUX_WDATA_REG_3 = 0x67, + SN_AUX_WDATA_REG_4 = 0x68, + SN_AUX_WDATA_REG_5 = 0x69, + SN_AUX_WDATA_REG_6 = 0x6A, + SN_AUX_WDATA_REG_7 = 0x6B, + SN_AUX_WDATA_REG_8 = 0x6C, + SN_AUX_WDATA_REG_9 = 0x6D, + SN_AUX_WDATA_REG_10 = 0x6E, + SN_AUX_WDATA_REG_11 = 0x6F, + SN_AUX_WDATA_REG_12 = 0x70, + SN_AUX_WDATA_REG_13 = 0x71, + SN_AUX_WDATA_REG_14 = 0x72, + SN_AUX_WDATA_REG_15 = 0x73, + SN_AUX_ADDR_19_16_REG = 0x74, + SN_AUX_ADDR_15_8_REG = 0x75, + SN_AUX_ADDR_7_0_REG = 0x76, + SN_AUX_LENGTH_REG = 0x77, + SN_AUX_CMD_REG = 0x78, + SN_AUX_RDATA_REG_0 = 0x79, + SN_AUX_RDATA_REG_1 = 0x7A, + SN_AUX_RDATA_REG_2 = 0x7B, + SN_AUX_RDATA_REG_3 = 0x7C, + SN_AUX_RDATA_REG_4 = 0x7D, + SN_AUX_RDATA_REG_5 = 0x7E, + SN_AUX_RDATA_REG_6 = 0x7F, + SN_AUX_RDATA_REG_7 = 0x80, + SN_AUX_RDATA_REG_8 = 0x81, + SN_AUX_RDATA_REG_9 = 0x82, + SN_AUX_RDATA_REG_10 = 0x83, + SN_AUX_RDATA_REG_11 = 0x84, + SN_AUX_RDATA_REG_12 = 0x85, + SN_AUX_RDATA_REG_13 = 0x86, + SN_AUX_RDATA_REG_14 = 0x87, + SN_AUX_RDATA_REG_15 = 0x88, + SN_SSC_CONFIG_REG = 0x93, + SN_DATARATE_CONFIG_REG = 0x94, + SN_ML_TX_MODE_REG = 0x96, + SN_AUX_CMD_STATUS_REG = 0xF4, +}; + +enum { + HPD_ENABLE = 0x0, + HPD_DISABLE = 0x1 , +}; + +enum { + SOT_ERR_TOL_DSI = 0x0, + CHB_DSI_LANES = 0x1, + CHA_DSI_LANES = 0x2, + DSI_CHANNEL_MODE = 0x3, + LEFT_RIGHT_PIXELS = 0x4, +}; + +enum vstream_config { + VSTREAM_DISABLE = 0, + VSTREAM_ENABLE = 1, +}; + +enum dp_pll_clk_src { + SN65_SEL_12MHZ = 0x0, + SN65_SEL_19MHZ = 0x1, + SN65_SEL_26MHZ = 0x2, + SN65_SEL_27MHZ = 0x3, + SN65_SEL_38MHZ = 0x4, +}; + +enum i2c_over_aux { + I2C_OVER_AUX_WRITE_MOT_0 = 0x0, + I2C_OVER_AUX_READ_MOT_0 = 0x1, + I2C_OVER_AUX_WRITE_STATUS_UPDATE_0 = 0x4, + I2C_OVER_AUX_WRITE_MOT_1 = 0x5, + I2C_OVER_AUX_READ_MOT_1 = 0x6, + I2C_OVER_AUX_WRITE_STATUS_UPDATE_1 = 0x7, + NATIVE_AUX_WRITE = 0x8, + NATIVE_AUX_READ = 0x9, +}; + +enum ml_tx_mode { + MAIN_LINK_OFF = 0x0, + NORMAL_MODE = 0x1, + TPS1 = 0x2, + TPS2 = 0x3, + TPS3 = 0x4, + PRBS7 = 0x5, + HBR2_COMPLIANCE_EYE_PATTERN = 0x6, + SYMBOL_ERR_RATE_MEASUREMENT_PATTERN = 0x7, + CUTSOM_PATTERN = 0x8, + FAST_LINK_TRAINING = 0x9, + SEMI_AUTO_LINK_TRAINING = 0xa, + REDRIVER_SEMI_AUTO_LINK_TRAINING = 0xb, +}; + +enum dpcd_request { + DPCD_READ = 0x0, + DPCD_WRITE = 0x1, +}; + +enum { + EDID_LENGTH = 128, + EDID_I2C_ADDR = 0x50, + I2C_CLAIM_ADDR_EN1 = 0x60, + EDID_EXTENSION_FLAG = 0x7e, +}; + +void sn65dsi86_bridge_init(uint8_t bus, uint8_t chip, + struct edid *edid, uint32_t num_of_lines, + uint8_t ref_clk, uint32_t bpp); +enum cb_err sn65dsi86_bridge_read_edid(uint8_t bus, uint8_t chip, struct edid *out); +#endif
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42899 )
Change subject: sc7180: Add support for sn65dsi86 bridge. ......................................................................
Patch Set 1:
(5 comments)
https://review.coreboot.org/c/coreboot/+/42899/1/src/drivers/ti/sn65dsi86bri... File src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.h:
https://review.coreboot.org/c/coreboot/+/42899/1/src/drivers/ti/sn65dsi86bri... PS1, Line 74: HPD_DISABLE = 0x1 , space prohibited before that ',' (ctx:WxE)
https://review.coreboot.org/c/coreboot/+/42899/1/src/drivers/ti/sn65dsi86bri... File src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c:
https://review.coreboot.org/c/coreboot/+/42899/1/src/drivers/ti/sn65dsi86bri... PS1, Line 245: if(dp_rate_idx < ARRAY_SIZE(sn65dsi86_bridge_dp_rate_lut)) space required before the open parenthesis '('
https://review.coreboot.org/c/coreboot/+/42899/1/src/drivers/ti/sn65dsi86bri... PS1, Line 343: i2c_write_field(bus, chip, SN_HPD_DISABLE_REG, HPD_DISABLE, 1, 0 ); space prohibited before that close parenthesis ')'
https://review.coreboot.org/c/coreboot/+/42899/1/src/drivers/ti/sn65dsi86bri... PS1, Line 381: sn65dsi86_bridge_set_dsi_clock_range(bus, chip, edid, num_of_lanes ,bpp); space prohibited before that ',' (ctx:WxV)
https://review.coreboot.org/c/coreboot/+/42899/1/src/drivers/ti/sn65dsi86bri... PS1, Line 381: sn65dsi86_bridge_set_dsi_clock_range(bus, chip, edid, num_of_lanes ,bpp); space required after that ',' (ctx:WxV)
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42899 )
Change subject: sc7180: Add support for sn65dsi86 bridge. ......................................................................
Patch Set 3:
(5 comments)
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... File src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.h:
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 74: HPD_DISABLE = 0x1 , space prohibited before that ',' (ctx:WxE)
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... File src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c:
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 245: if(dp_rate_idx < ARRAY_SIZE(sn65dsi86_bridge_dp_rate_lut)) space required before the open parenthesis '('
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 343: i2c_write_field(bus, chip, SN_HPD_DISABLE_REG, HPD_DISABLE, 1, 0 ); space prohibited before that close parenthesis ')'
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 381: sn65dsi86_bridge_set_dsi_clock_range(bus, chip, edid, num_of_lanes ,bpp); space prohibited before that ',' (ctx:WxV)
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 381: sn65dsi86_bridge_set_dsi_clock_range(bus, chip, edid, num_of_lanes ,bpp); space required after that ',' (ctx:WxV)
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42899 )
Change subject: sc7180: Add support for sn65dsi86 bridge. ......................................................................
Patch Set 3:
(19 comments)
https://review.coreboot.org/c/coreboot/+/42899/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42899/3//COMMIT_MSG@7 PS3, Line 7: sc7180: Add support for sn65dsi86 bridge. Please remove the dot/period at the end of the commit message.
https://review.coreboot.org/c/coreboot/+/42899/3//COMMIT_MSG@10 PS3, Line 10: Please mention the datasheet name and revision.
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... File src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c:
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 13: #define bridge_debug(x...) do {if (0) printk(BIOS_DEBUG, x); } while (0) Please add a separate debug macro. Please look how other drivers do it.
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 15: #define BRIDGE_GETHIGHERBYTE(x) (uint8_t)((x&0xFF00)>>8) Please add spaces around the operator and use lowercase hex letters.
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 47: * by the bridge in Mbps unit. Fits on the line above?
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 59: Disable disable
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 97: int unsigned int
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 141: int unsigned int
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 219: int size_t or unsigned_int?
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 224: int unsigned int?
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 248: printk(BIOS_ERR, "ERROR: valid dp rate not found"); Maybe:
No valid dp rate found.
What is the consequence for the user?
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 291: printk(BIOS_ERR, "ERROR: PLL lock failure\n"); … after 500 ms.
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 310: printk(BIOS_ERR, "ERROR: Link training failed");
… after 500 ms timeout.
What is the consequence for the user? Display might not work?
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 329: version versions
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 332: to compatible to be
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 327: /* : * support bridge HPD function : * some hardware version do not support bridge hdp, : * we use 360ms to try to get the hpd single now, : * if we can not get bridge hpd single, it will delay 360ms, : * also meet the bridge power timing request, to compatible : * all of the hardware version : */ Please re-flow for 96 characters.
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 337: 360 360 ms is very long for coreboot.
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 385: /* Disable vstream */ Why should it be disabled?
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 393: /* color bar disabled */
Disable color bar
What is a color bar?
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42899 )
Change subject: sc7180: Add support for sn65dsi86 bridge. ......................................................................
Patch Set 3:
Can’t the coreboot EDID helpers be used?
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42899 )
Change subject: sc7180: Add support for sn65dsi86 bridge. ......................................................................
Patch Set 3:
(5 comments)
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... File src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.h:
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 74: HPD_DISABLE = 0x1 , space prohibited before that ',' (ctx:WxE)
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... File src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c:
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 245: if(dp_rate_idx < ARRAY_SIZE(sn65dsi86_bridge_dp_rate_lut)) space required before the open parenthesis '('
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 343: i2c_write_field(bus, chip, SN_HPD_DISABLE_REG, HPD_DISABLE, 1, 0 ); space prohibited before that close parenthesis ')'
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 381: sn65dsi86_bridge_set_dsi_clock_range(bus, chip, edid, num_of_lanes ,bpp); space prohibited before that ',' (ctx:WxV)
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 381: sn65dsi86_bridge_set_dsi_clock_range(bus, chip, edid, num_of_lanes ,bpp); space required after that ',' (ctx:WxV)
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42899 )
Change subject: sc7180: Add support for sn65dsi86 bridge. ......................................................................
Patch Set 4:
(5 comments)
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... File src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.h:
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... PS4, Line 74: HPD_DISABLE = 0x1 , space prohibited before that ',' (ctx:WxE)
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... File src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c:
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... PS4, Line 245: if(dp_rate_idx < ARRAY_SIZE(sn65dsi86_bridge_dp_rate_lut)) space required before the open parenthesis '('
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... PS4, Line 343: i2c_write_field(bus, chip, SN_HPD_DISABLE_REG, HPD_DISABLE, 1, 0 ); space prohibited before that close parenthesis ')'
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... PS4, Line 381: sn65dsi86_bridge_set_dsi_clock_range(bus, chip, edid, num_of_lanes ,bpp); space prohibited before that ',' (ctx:WxV)
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... PS4, Line 381: sn65dsi86_bridge_set_dsi_clock_range(bus, chip, edid, num_of_lanes ,bpp); space required after that ',' (ctx:WxV)
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42899 )
Change subject: sc7180: Add support for sn65dsi86 bridge. ......................................................................
Patch Set 4:
(21 comments)
Can’t the coreboot EDID helpers be used?
Not sure what you mean, this is already calling decode_edid(). Is there any other common code it should be using? (The whole "read EDID over I2C" thing has a bunch of common code that would be nice to factor out at some point, e.g. a couple of lines here are straight out copied from the ps8640 driver. But I'm not sure it's fair to tie that work onto this CL.)
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... File src/drivers/ti/sn65dsi86bridge/Kconfig:
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... PS4, Line 5: y Drivers should always be 'default n'. You should put a 'select DRIVERS_TI_SN65DSI86BRIDGE' under BOARD_SPECIFIC_OPTIONS in src/mainboard/google/trogdor/Kconfig.
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... PS4, Line 7: TI SN65DSI86BRIDGE Please write at least one full sentence.
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... File src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.h:
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... PS4, Line 8: enum bridge_regs { Please but all these enums into sn65dsi86bridge.c, not sn65dsi86bridge.h, so you don't pollute the namespace of files which are just including this to call the functions.
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... PS4, Line 90: enum dp_pll_clk_src { (This is the one enum that needs to stay in the sn65dsi86bridge.h file because it needs to be passed in as a parameter. That's okay because it's prefixed SN65_, unlike the others.)
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... PS4, Line 132: I2C_CLAIM_ADDR_EN1 = 0x60, This is a bridge register, shouldn't it be further up with the others and called SN_I2C_CLAIM_ADDR_EN1?
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... PS4, Line 138: uint8_t You should write 'enum dp_pll_clk_src' instead of uint8_t here to make it clearer what the argument expects.
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... PS4, Line 139: enum cb_err sn65dsi86_bridge_read_edid(uint8_t bus, uint8_t chip, struct edid *out); Blank line before #endif (please don't say "Done" on comments you didn't actually address without any further explanation).
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... File src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c:
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 13: #define bridge_debug(x...) do {if (0) printk(BIOS_DEBUG, x); } while (0)
Please add a separate debug macro. Please look how other drivers do it.
This isn't used anyway, let's just drop it?
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 15: #define BRIDGE_GETHIGHERBYTE(x) (uint8_t)((x&0xFF00)>>8)
Please add spaces around the operator and use lowercase hex letters.
I'm not aware of any rule forcing a certain capitalization of hex letters?
This needs parentheses around the whole expression, though.
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 97: int
unsigned int
Why? I'm not aware of any rules on signedness either (unless we're doing MISRA now... oh god please no!). For simple loop variables and other stuff that clearly fits in the signed data type I see no reason not do use 'int' (in some cases, like for loops running from high to low, using signed types can actually prevent common mistakes).
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 248: printk(BIOS_ERR, "ERROR: valid dp rate not found");
Maybe: […]
Display doesn't work, probably?
I guess we could argue whether we want to bubble up error values throughout everything here, though I'm not sure if it's worth making the code more complicated for it. Either way it's gonna result in the display not working (doing further display init stuff when this fails would probably be harmless, other than maybe printing some further errors).
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 385: /* Disable vstream */
Why should it be disabled?
I think the idea is to turn it of, initialize everything correctly and then turn it on again to avoid visual glitches appearing on the screen during initialization?
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 393: /* color bar disabled */
Disable color bar […]
A test pattern the bridge generates by default when you turn it on. You can read the whole datasheet here if you want: https://www.ti.com/lit/ds/sllseh2b/sllseh2b.pdf
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... File src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c:
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... PS4, Line 60: i2c_writeb(bus, chip, SN_HPD_DISABLE_REG, 0x1); This doesn't seem thematically related to reading the EDID. Does this really have to be here (as opposed to in sn65dsi86_bridge_init())?
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... PS4, Line 150: ARRAY_SIZE This read is in bytes, not items. So while it makes no difference for uint8_t, it would be better to write sizeof() here.
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... PS4, Line 152: ARRAY_SIZE (Here ARRAY_SIZE() is the correct one, btw.)
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... PS4, Line 158: rate_mhz = rate_per_200khz * 200 / 1000; Are we okay with possible inaccuracy from rounding here? Since it looks like you're trying to check for exact equality, I think
for (j = 0; j < ARRAY_SIZE(...); j++) if (...lut[j] * (MHz/KHz) == rate_per_200khz * 200) rate_valid[j] = true;
would be more correct.
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... PS4, Line 205: 1000 Please use the constants KHz (1000) and MHz (1000000) instead of raw numbers where appropriate.
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... PS4, Line 206: stream_bit_rate_mhz = (pixel_clk_hz / 1000000) * bpp; Not sure how accurate you want this but maybe write
pixel_clk_hz * bpp / MHz
?
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... PS4, Line 212: (MIN_DSI_CLK_FREQ_MHZ / 5) + : (((min_req_dsi_clk - MIN_DSI_CLK_FREQ_MHZ) / 5) & 0xFF) Maybe I'm missing something but isn't this just a more complicated way to say min_req_dsi_clk / 5?
Also, the datasheet says the valid range is 40 to 750 so maybe write
min_req_dsi_clk = MIN(MIN_DSI_CLK_FREQ_MHZ, MAX(MAX_DSI_CLK_FREQ_MHZ, min_req_dsi_clk)) / 5;
(with MAX_DSI_CLK_FREQ_MHZ being 750... maybe the name could be a bit shorter too).
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... PS4, Line 391: (uint8_t) Why is there a cast here?
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42899 )
Change subject: sc7180: Add support for sn65dsi86 bridge. ......................................................................
Patch Set 5:
(5 comments)
https://review.coreboot.org/c/coreboot/+/42899/5/src/drivers/ti/sn65dsi86bri... File src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.h:
https://review.coreboot.org/c/coreboot/+/42899/5/src/drivers/ti/sn65dsi86bri... PS5, Line 74: HPD_DISABLE = 0x1 , space prohibited before that ',' (ctx:WxE)
https://review.coreboot.org/c/coreboot/+/42899/5/src/drivers/ti/sn65dsi86bri... File src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c:
https://review.coreboot.org/c/coreboot/+/42899/5/src/drivers/ti/sn65dsi86bri... PS5, Line 245: if(dp_rate_idx < ARRAY_SIZE(sn65dsi86_bridge_dp_rate_lut)) space required before the open parenthesis '('
https://review.coreboot.org/c/coreboot/+/42899/5/src/drivers/ti/sn65dsi86bri... PS5, Line 343: i2c_write_field(bus, chip, SN_HPD_DISABLE_REG, HPD_DISABLE, 1, 0 ); space prohibited before that close parenthesis ')'
https://review.coreboot.org/c/coreboot/+/42899/5/src/drivers/ti/sn65dsi86bri... PS5, Line 381: sn65dsi86_bridge_set_dsi_clock_range(bus, chip, edid, num_of_lanes ,bpp); space prohibited before that ',' (ctx:WxV)
https://review.coreboot.org/c/coreboot/+/42899/5/src/drivers/ti/sn65dsi86bri... PS5, Line 381: sn65dsi86_bridge_set_dsi_clock_range(bus, chip, edid, num_of_lanes ,bpp); space required after that ',' (ctx:WxV)
Vinod Polimera has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42899 )
Change subject: sc7180: Add support for sn65dsi86 bridge. ......................................................................
Patch Set 5:
(28 comments)
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... File src/drivers/ti/sn65dsi86bridge/Kconfig:
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... PS4, Line 5: y
Drivers should always be 'default n'. […]
Done
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... PS4, Line 7: TI SN65DSI86BRIDGE
Please write at least one full sentence.
Done
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... File src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.h:
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... PS4, Line 8: enum bridge_regs {
Please but all these enums into sn65dsi86bridge.c, not sn65dsi86bridge. […]
Done
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... PS4, Line 90: enum dp_pll_clk_src {
(This is the one enum that needs to stay in the sn65dsi86bridge. […]
Done
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... PS4, Line 132: I2C_CLAIM_ADDR_EN1 = 0x60,
This is a bridge register, shouldn't it be further up with the others and called SN_I2C_CLAIM_ADDR_E […]
Done
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... PS4, Line 138: uint8_t
You should write 'enum dp_pll_clk_src' instead of uint8_t here to make it clearer what the argument […]
Done
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... PS4, Line 139: enum cb_err sn65dsi86_bridge_read_edid(uint8_t bus, uint8_t chip, struct edid *out);
Blank line before #endif (please don't say "Done" on comments you didn't actually address without an […]
Done
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... File src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c:
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 13: #define bridge_debug(x...) do {if (0) printk(BIOS_DEBUG, x); } while (0)
This isn't used anyway, let's just drop it?
Done
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 15: #define BRIDGE_GETHIGHERBYTE(x) (uint8_t)((x&0xFF00)>>8)
I'm not aware of any rule forcing a certain capitalization of hex letters? […]
Done
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 47: * by the bridge in Mbps unit.
Fits on the line above?
Done
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 59: Disable
disable
Done
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 219: int
size_t or unsigned_int?
Done
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 291: printk(BIOS_ERR, "ERROR: PLL lock failure\n");
… after 500 ms.
Display will not work
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 310: printk(BIOS_ERR, "ERROR: Link training failed");
… after 500 ms timeout. […]
Yes , Display will not work
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 329: version
versions
Done
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 332: to compatible
to be
Done
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 327: /* : * support bridge HPD function : * some hardware version do not support bridge hdp, : * we use 360ms to try to get the hpd single now, : * if we can not get bridge hpd single, it will delay 360ms, : * also meet the bridge power timing request, to compatible : * all of the hardware version : */
Please re-flow for 96 characters.
Done
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 337: 360
360 ms is very long for coreboot.
Datasheet says to wait for max 400 ms
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 385: /* Disable vstream */
I think the idea is to turn it of, initialize everything correctly and then turn it on again to avoi […]
Done
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 393: /* color bar disabled */
A test pattern the bridge generates by default when you turn it on. […]
Done
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... File src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c:
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... PS4, Line 60: i2c_writeb(bus, chip, SN_HPD_DISABLE_REG, 0x1);
This doesn't seem thematically related to reading the EDID. […]
Done
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... PS4, Line 150: ARRAY_SIZE
This read is in bytes, not items. […]
Done
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... PS4, Line 152: ARRAY_SIZE
(Here ARRAY_SIZE() is the correct one, btw. […]
Done
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... PS4, Line 158: rate_mhz = rate_per_200khz * 200 / 1000;
Are we okay with possible inaccuracy from rounding here? Since it looks like you're trying to check […]
Done
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... PS4, Line 205: 1000
Please use the constants KHz (1000) and MHz (1000000) instead of raw numbers where appropriate.
Done
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... PS4, Line 206: stream_bit_rate_mhz = (pixel_clk_hz / 1000000) * bpp;
Not sure how accurate you want this but maybe write […]
Done
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... PS4, Line 212: (MIN_DSI_CLK_FREQ_MHZ / 5) + : (((min_req_dsi_clk - MIN_DSI_CLK_FREQ_MHZ) / 5) & 0xFF)
Maybe I'm missing something but isn't this just a more complicated way to say min_req_dsi_clk / 5? […]
Done
https://review.coreboot.org/c/coreboot/+/42899/4/src/drivers/ti/sn65dsi86bri... PS4, Line 391: (uint8_t)
Why is there a cast here?
Done
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42899 )
Change subject: sc7180: Add support for sn65dsi86 bridge. ......................................................................
Patch Set 6:
(5 comments)
https://review.coreboot.org/c/coreboot/+/42899/6/src/drivers/ti/sn65dsi86bri... File src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.h:
https://review.coreboot.org/c/coreboot/+/42899/6/src/drivers/ti/sn65dsi86bri... PS6, Line 74: HPD_DISABLE = 0x1 , space prohibited before that ',' (ctx:WxE)
https://review.coreboot.org/c/coreboot/+/42899/6/src/drivers/ti/sn65dsi86bri... File src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c:
https://review.coreboot.org/c/coreboot/+/42899/6/src/drivers/ti/sn65dsi86bri... PS6, Line 245: if(dp_rate_idx < ARRAY_SIZE(sn65dsi86_bridge_dp_rate_lut)) space required before the open parenthesis '('
https://review.coreboot.org/c/coreboot/+/42899/6/src/drivers/ti/sn65dsi86bri... PS6, Line 343: i2c_write_field(bus, chip, SN_HPD_DISABLE_REG, HPD_DISABLE, 1, 0 ); space prohibited before that close parenthesis ')'
https://review.coreboot.org/c/coreboot/+/42899/6/src/drivers/ti/sn65dsi86bri... PS6, Line 381: sn65dsi86_bridge_set_dsi_clock_range(bus, chip, edid, num_of_lanes ,bpp); space prohibited before that ',' (ctx:WxV)
https://review.coreboot.org/c/coreboot/+/42899/6/src/drivers/ti/sn65dsi86bri... PS6, Line 381: sn65dsi86_bridge_set_dsi_clock_range(bus, chip, edid, num_of_lanes ,bpp); space required after that ',' (ctx:WxV)
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42899 )
Change subject: sc7180: Add support for sn65dsi86 bridge. ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... File src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c:
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 337: 360
Datasheet says to wait for max 400 ms
Then let's wait 400ms instead of 360ms?
In general I think this is fine since this is only a timeout. I expect that in most "normal" cases it will be a lot faster. I'm more worried about those plain mdelay() that we have in some places.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42899
to look at the new patch set (#7).
Change subject: sc7180: Add support for sn65dsi86 bridge. ......................................................................
sc7180: Add support for sn65dsi86 bridge.
Add sn65dsi86 bridge driver to enable the eDP bridge.
Change-Id: I36a68f3241f0ba316c261a73c2f6d30fe6c3ccdc Signed-off-by: Vinod Polimera vpolimer@codeaurora.org --- A src/drivers/ti/sn65dsi86bridge/Kconfig A src/drivers/ti/sn65dsi86bridge/Makefile.inc A src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c A src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.h 4 files changed, 540 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/42899/7
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42899 )
Change subject: sc7180: Add support for sn65dsi86 bridge. ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42899/7/src/drivers/ti/sn65dsi86bri... File src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c:
https://review.coreboot.org/c/coreboot/+/42899/7/src/drivers/ti/sn65dsi86bri... PS7, Line 470: data As discussed over email, this needs to be masked with a DP_LANE_COUNT_MASK (0xf).
https://review.coreboot.org/c/coreboot/+/42899/7/src/drivers/ti/sn65dsi86bri... PS7, Line 484: i2c_write_field(bus, chip, SN_DPPLL_SRC_REG, ref_clk, 7, 1); As discussed over email, this (and probably HPD) should be factored out into a separate function that has to be called before reading the EDID.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42899 )
Change subject: sc7180: Add support for sn65dsi86 bridge. ......................................................................
Patch Set 7:
Patch Set 4:
(21 comments)
Can’t the coreboot EDID helpers be used?
Not sure what you mean, this is already calling decode_edid(). Is there any other common code it should be using? (The whole "read EDID over I2C" thing has a bunch of common code that would be nice to factor out at some point, e.g. a couple of lines here are straight out copied from the ps8640 driver. But I'm not sure it's fair to tie that work onto this CL.)
Ah, my bad. I thought there was already common code for that.
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/+/42899
to look at the new patch set (#10).
Change subject: Sc7180: Add support for sn65dsi86 bridge ......................................................................
Sc7180: Add support for sn65dsi86 bridge
Add sn65dsi86 bridge driver to enable the eDP bridge. Datasheet used : https://www.ti.com/lit/ds/sllseh2b/sllseh2b.pdf
Changes in V1: - fix the dp lanes using mask - separate out the refclk and hpd config to init function
Change-Id: I36a68f3241f0ba316c261a73c2f6d30fe6c3ccdc Signed-off-by: Vinod Polimera vpolimer@codeaurora.org --- A src/drivers/ti/sn65dsi86bridge/Kconfig A src/drivers/ti/sn65dsi86bridge/Makefile.inc A src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c A src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.h M src/mainboard/google/trogdor/Kconfig 5 files changed, 546 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/42899/10
Vinod Polimera has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42899 )
Change subject: Sc7180: Add support for sn65dsi86 bridge ......................................................................
Patch Set 10:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42899/7/src/drivers/ti/sn65dsi86bri... File src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c:
https://review.coreboot.org/c/coreboot/+/42899/7/src/drivers/ti/sn65dsi86bri... PS7, Line 470: data
As discussed over email, this needs to be masked with a DP_LANE_COUNT_MASK (0xf).
Done
https://review.coreboot.org/c/coreboot/+/42899/7/src/drivers/ti/sn65dsi86bri... PS7, Line 484: i2c_write_field(bus, chip, SN_DPPLL_SRC_REG, ref_clk, 7, 1);
As discussed over email, this (and probably HPD) should be factored out into a separate function tha […]
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42899 )
Change subject: Sc7180: Add support for sn65dsi86 bridge ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42899/10//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42899/10//COMMIT_MSG@7 PS10, Line 7: Sc7180 All other prefixes use lowercase.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42899 )
Change subject: Sc7180: Add support for sn65dsi86 bridge ......................................................................
Patch Set 10: Code-Review+2
(9 comments)
LGTM after resolving outstanding comments
https://review.coreboot.org/c/coreboot/+/42899/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42899/3//COMMIT_MSG@7 PS3, Line 7: sc7180: Add support for sn65dsi86 bridge.
Please remove the dot/period at the end of the commit message.
Done
https://review.coreboot.org/c/coreboot/+/42899/10//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42899/10//COMMIT_MSG@7 PS10, Line 7: Sc7180
All other prefixes use lowercase.
still outstanding
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... File src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c:
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 97: int
Why? I'm not aware of any rules on signedness either (unless we're doing MISRA now... […]
Assuming resolved.
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 141: int
unsigned int
Assuming resolved.
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 224: int
unsigned int?
Assuming resolved.
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 248: printk(BIOS_ERR, "ERROR: valid dp rate not found");
Display doesn't work, probably? […]
Assuming resolved.
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 291: printk(BIOS_ERR, "ERROR: PLL lock failure\n");
Display will not work
Assuming resolved.
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 310: printk(BIOS_ERR, "ERROR: Link training failed");
Yes , Display will not work
Assuming resolved.
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 337: 360
Then let's wait 400ms instead of 360ms? […]
still outstanding (if the datasheet says 400 we should use 400, right?)
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/+/42899
to look at the new patch set (#12).
Change subject: Sc7180: Add support for sn65dsi86 bridge ......................................................................
Sc7180: Add support for sn65dsi86 bridge
Add sn65dsi86 bridge driver to enable the eDP bridge. Datasheet used : https://www.ti.com/lit/ds/sllseh2b/sllseh2b.pdf
Changes in V1: - fix the dp lanes using mask - separate out the refclk and hpd config to init function
Change-Id: I36a68f3241f0ba316c261a73c2f6d30fe6c3ccdc Signed-off-by: Vinod Polimera vpolimer@codeaurora.org --- A src/drivers/ti/sn65dsi86bridge/Kconfig A src/drivers/ti/sn65dsi86bridge/Makefile.inc A src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c A src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.h M src/mainboard/google/trogdor/Kconfig 5 files changed, 546 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/42899/12
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42899 )
Change subject: Sc7180: Add support for sn65dsi86 bridge ......................................................................
Patch Set 12: Code-Review+2
(3 comments)
https://review.coreboot.org/c/coreboot/+/42899/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42899/3//COMMIT_MSG@10 PS3, Line 10:
Please mention the datasheet name and revision.
Done
https://review.coreboot.org/c/coreboot/+/42899/10//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42899/10//COMMIT_MSG@7 PS10, Line 7: Sc7180
still outstanding
*ping*
Please fix subject.
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... File src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c:
https://review.coreboot.org/c/coreboot/+/42899/3/src/drivers/ti/sn65dsi86bri... PS3, Line 337: 360
still outstanding (if the datasheet says 400 we should use 400, right?)
Done
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42899 )
Change subject: Sc7180: Add support for sn65dsi86 bridge ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42899/12/src/drivers/ti/sn65dsi86br... File src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c:
https://review.coreboot.org/c/coreboot/+/42899/12/src/drivers/ti/sn65dsi86br... PS12, Line 473: return data; As discussed on https://issuetracker.google.com/issues/161373813, this return value also needs to be masked. Should just do something like
uint8_t lane_count;
sn65dsi86_bridge_dpcd_request(bus, chip, DP_MAX_LANE_COUNT, 1, DPCD_READ, &data); late_count &= DP_LANE_COUNT_MASK; i2c_write_field(bus, chip, SN_SSC_CONFIG_REG, MIN(lane_count, 3), 3, 4);
return lane_count;
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/+/42899
to look at the new patch set (#13).
Change subject: sc7180: Add support for sn65dsi86 bridge ......................................................................
sc7180: Add support for sn65dsi86 bridge
Add sn65dsi86 bridge driver to enable the eDP bridge. Datasheet used : https://www.ti.com/lit/ds/sllseh2b/sllseh2b.pdf
Changes in V1: - fix the dp lanes using mask - separate out the refclk and hpd config to init function
Change-Id: I36a68f3241f0ba316c261a73c2f6d30fe6c3ccdc Signed-off-by: Vinod Polimera vpolimer@codeaurora.org --- A src/drivers/ti/sn65dsi86bridge/Kconfig A src/drivers/ti/sn65dsi86bridge/Makefile.inc A src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c A src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.h M src/mainboard/google/trogdor/Kconfig 5 files changed, 547 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/42899/13
Vinod Polimera has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42899 )
Change subject: sc7180: Add support for sn65dsi86 bridge ......................................................................
Patch Set 13:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42899/10//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42899/10//COMMIT_MSG@7 PS10, Line 7: Sc7180
*ping* […]
Done
https://review.coreboot.org/c/coreboot/+/42899/12/src/drivers/ti/sn65dsi86br... File src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c:
https://review.coreboot.org/c/coreboot/+/42899/12/src/drivers/ti/sn65dsi86br... PS12, Line 473: return data;
As discussed on https://issuetracker.google. […]
Done
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42899 )
Change subject: sc7180: Add support for sn65dsi86 bridge ......................................................................
Patch Set 13: Code-Review+2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42899 )
Change subject: sc7180: Add support for sn65dsi86 bridge ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42899/17/src/drivers/ti/sn65dsi86br... File src/drivers/ti/sn65dsi86bridge/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/42899/17/src/drivers/ti/sn65dsi86br... PS17, Line 3: ramstage-$(CONFIG_DRIVERS_TI_SN65DSI86BRIDGE) += sn65dsi86bridge.c There needs to be a newline at the end of this file or the bot won't verify the patch.
mturney mturney has uploaded a new patch set (#18) to the change originally created by Ravi kumar. ( https://review.coreboot.org/c/coreboot/+/42899 )
Change subject: sc7180: Add support for sn65dsi86 bridge ......................................................................
sc7180: Add support for sn65dsi86 bridge
Add sn65dsi86 bridge driver to enable the eDP bridge. Datasheet used : https://www.ti.com/lit/ds/sllseh2b/sllseh2b.pdf
Changes in V1: - fix the dp lanes using mask - separate out the refclk and hpd config to init function
Change-Id: I36a68f3241f0ba316c261a73c2f6d30fe6c3ccdc Signed-off-by: Vinod Polimera vpolimer@codeaurora.org --- A src/drivers/ti/sn65dsi86bridge/Kconfig A src/drivers/ti/sn65dsi86bridge/Makefile.inc A src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c A src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.h M src/mainboard/google/trogdor/Kconfig 5 files changed, 548 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/42899/18
mturney mturney has uploaded a new patch set (#19) to the change originally created by Ravi kumar. ( https://review.coreboot.org/c/coreboot/+/42899 )
Change subject: sc7180: Add support for sn65dsi86 bridge ......................................................................
sc7180: Add support for sn65dsi86 bridge
Add sn65dsi86 bridge driver to enable the eDP bridge. Datasheet used : https://www.ti.com/lit/ds/sllseh2b/sllseh2b.pdf
Changes in V1: - fix the dp lanes using mask - separate out the refclk and hpd config to init function
Change-Id: I36a68f3241f0ba316c261a73c2f6d30fe6c3ccdc Signed-off-by: Vinod Polimera vpolimer@codeaurora.org --- A src/drivers/ti/sn65dsi86bridge/Kconfig A src/drivers/ti/sn65dsi86bridge/Makefile.inc A src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c A src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.h M src/mainboard/google/trogdor/Kconfig 5 files changed, 547 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/42899/19
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42899 )
Change subject: sc7180: Add support for sn65dsi86 bridge ......................................................................
Patch Set 19: Code-Review+2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42899 )
Change subject: sc7180: Add support for sn65dsi86 bridge ......................................................................
Patch Set 19:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42899/17/src/drivers/ti/sn65dsi86br... File src/drivers/ti/sn65dsi86bridge/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/42899/17/src/drivers/ti/sn65dsi86br... PS17, Line 3: ramstage-$(CONFIG_DRIVERS_TI_SN65DSI86BRIDGE) += sn65dsi86bridge.c
There needs to be a newline at the end of this file or the bot won't verify the patch.
Done
Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42899 )
Change subject: sc7180: Add support for sn65dsi86 bridge ......................................................................
sc7180: Add support for sn65dsi86 bridge
Add sn65dsi86 bridge driver to enable the eDP bridge. Datasheet used : https://www.ti.com/lit/ds/sllseh2b/sllseh2b.pdf
Changes in V1: - fix the dp lanes using mask - separate out the refclk and hpd config to init function
Change-Id: I36a68f3241f0ba316c261a73c2f6d30fe6c3ccdc Signed-off-by: Vinod Polimera vpolimer@codeaurora.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/42899 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Julius Werner jwerner@chromium.org --- A src/drivers/ti/sn65dsi86bridge/Kconfig A src/drivers/ti/sn65dsi86bridge/Makefile.inc A src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c A src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.h M src/mainboard/google/trogdor/Kconfig 5 files changed, 547 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved
diff --git a/src/drivers/ti/sn65dsi86bridge/Kconfig b/src/drivers/ti/sn65dsi86bridge/Kconfig new file mode 100644 index 0000000..b7abb34 --- /dev/null +++ b/src/drivers/ti/sn65dsi86bridge/Kconfig @@ -0,0 +1,7 @@ +## SPDX-License-Identifier: GPL-2.0-only + +config DRIVERS_TI_SN65DSI86BRIDGE + bool + default n + help + TI SN65DSI86 eDP bridge driver diff --git a/src/drivers/ti/sn65dsi86bridge/Makefile.inc b/src/drivers/ti/sn65dsi86bridge/Makefile.inc new file mode 100644 index 0000000..b146fe3 --- /dev/null +++ b/src/drivers/ti/sn65dsi86bridge/Makefile.inc @@ -0,0 +1,3 @@ +## SPDX-License-Identifier: GPL-2.0-only + +ramstage-$(CONFIG_DRIVERS_TI_SN65DSI86BRIDGE) += sn65dsi86bridge.c diff --git a/src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c b/src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c new file mode 100644 index 0000000..e0058c4 --- /dev/null +++ b/src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c @@ -0,0 +1,514 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <console/console.h> +#include <delay.h> +#include <endian.h> +#include <device/i2c_simple.h> +#include <edid.h> +#include <timer.h> +#include <types.h> +#include <soc/addressmap.h> +#include "sn65dsi86bridge.h" + +#define BRIDGE_GETHIGHERBYTE(x) (uint8_t)((x & 0xff00) >> 8) +#define BRIDGE_GETLOWERBYTE(x) (uint8_t)((x & 0x00ff)) + +/* fudge factor required to account for 8b/10b encoding */ +#define DP_CLK_FUDGE_NUM 10 +#define DP_CLK_FUDGE_DEN 8 + +/* DPCD */ +#define DP_BRIDGE_DPCD_REV 0x700 +#define DP_BRIDGE_11 0x00 +#define DP_BRIDGE_12 0x01 +#define DP_BRIDGE_13 0x02 +#define DP_BRIDGE_14 0x03 +#define DP_BRIDGE_CONFIGURATION_SET 0x10a +#define DP_MAX_LINK_RATE 0x001 +#define DP_MAX_LANE_COUNT 0x002 +#define DP_SUPPORTED_LINK_RATES 0x010 /* eDP 1.4 */ +#define DP_MAX_LINK_RATE 0x001 +#define DP_MAX_SUPPORTED_RATES 8 /* 16-bit little-endian */ +#define DP_LANE_COUNT_MASK 0xf + +/* link configuration */ +#define DP_LINK_BW_SET 0x100 +#define DP_LINK_BW_1_62 0x06 +#define DP_LINK_BW_2_7 0x0a +#define DP_LINK_BW_5_4 0x14 + +#define AUX_CMD_SEND 0x1 +#define MIN_DSI_CLK_FREQ_MHZ 40 +#define MAX_DSI_CLK_FREQ_MHZ 750 + +enum bridge_regs { + SN_DPPLL_SRC_REG = 0x0A, + SN_PLL_ENABLE_REG = 0x0D, + SN_DSI_LANES_REG = 0x10, + SN_DSIA_CLK_FREQ_REG = 0x12, + SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG = 0x20, + SN_CHA_ACTIVE_LINE_LENGTH_HIGH_REG = 0x21, + SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG = 0x24, + SN_CHA_VERTICAL_DISPLAY_SIZE_HIGH_REG = 0x25, + SN_CHA_HSYNC_PULSE_WIDTH_LOW_REG = 0x2C, + SN_CHA_HSYNC_PULSE_WIDTH_HIGH_REG = 0x2D, + SN_CHA_VSYNC_PULSE_WIDTH_LOW_REG = 0x30, + SN_CHA_VSYNC_PULSE_WIDTH_HIGH_REG = 0x31, + SN_CHA_HORIZONTAL_BACK_PORCH_REG = 0x34, + SN_CHA_VERTICAL_BACK_PORCH_REG = 0x36, + SN_CHA_HORIZONTAL_FRONT_PORCH_REG = 0x38, + SN_CHA_VERTICAL_FRONT_PORCH_REG = 0x3A, + SN_COLOR_BAR_REG = 0x3C, + SN_ENH_FRAME_REG = 0x5A, + SN_DATA_FORMAT_REG = 0x5B, + SN_HPD_DISABLE_REG = 0x5C, + SN_I2C_CLAIM_ADDR_EN1 = 0x60, + SN_AUX_WDATA_REG_0 = 0x64, + SN_AUX_WDATA_REG_1 = 0x65, + SN_AUX_WDATA_REG_2 = 0x66, + SN_AUX_WDATA_REG_3 = 0x67, + SN_AUX_WDATA_REG_4 = 0x68, + SN_AUX_WDATA_REG_5 = 0x69, + SN_AUX_WDATA_REG_6 = 0x6A, + SN_AUX_WDATA_REG_7 = 0x6B, + SN_AUX_WDATA_REG_8 = 0x6C, + SN_AUX_WDATA_REG_9 = 0x6D, + SN_AUX_WDATA_REG_10 = 0x6E, + SN_AUX_WDATA_REG_11 = 0x6F, + SN_AUX_WDATA_REG_12 = 0x70, + SN_AUX_WDATA_REG_13 = 0x71, + SN_AUX_WDATA_REG_14 = 0x72, + SN_AUX_WDATA_REG_15 = 0x73, + SN_AUX_ADDR_19_16_REG = 0x74, + SN_AUX_ADDR_15_8_REG = 0x75, + SN_AUX_ADDR_7_0_REG = 0x76, + SN_AUX_LENGTH_REG = 0x77, + SN_AUX_CMD_REG = 0x78, + SN_AUX_RDATA_REG_0 = 0x79, + SN_AUX_RDATA_REG_1 = 0x7A, + SN_AUX_RDATA_REG_2 = 0x7B, + SN_AUX_RDATA_REG_3 = 0x7C, + SN_AUX_RDATA_REG_4 = 0x7D, + SN_AUX_RDATA_REG_5 = 0x7E, + SN_AUX_RDATA_REG_6 = 0x7F, + SN_AUX_RDATA_REG_7 = 0x80, + SN_AUX_RDATA_REG_8 = 0x81, + SN_AUX_RDATA_REG_9 = 0x82, + SN_AUX_RDATA_REG_10 = 0x83, + SN_AUX_RDATA_REG_11 = 0x84, + SN_AUX_RDATA_REG_12 = 0x85, + SN_AUX_RDATA_REG_13 = 0x86, + SN_AUX_RDATA_REG_14 = 0x87, + SN_AUX_RDATA_REG_15 = 0x88, + SN_SSC_CONFIG_REG = 0x93, + SN_DATARATE_CONFIG_REG = 0x94, + SN_ML_TX_MODE_REG = 0x96, + SN_AUX_CMD_STATUS_REG = 0xF4, +}; + +enum { + HPD_ENABLE = 0x0, + HPD_DISABLE = 0x1, +}; + +enum { + SOT_ERR_TOL_DSI = 0x0, + CHB_DSI_LANES = 0x1, + CHA_DSI_LANES = 0x2, + DSI_CHANNEL_MODE = 0x3, + LEFT_RIGHT_PIXELS = 0x4, +}; + +enum vstream_config { + VSTREAM_DISABLE = 0, + VSTREAM_ENABLE = 1, +}; + +enum i2c_over_aux { + I2C_OVER_AUX_WRITE_MOT_0 = 0x0, + I2C_OVER_AUX_READ_MOT_0 = 0x1, + I2C_OVER_AUX_WRITE_STATUS_UPDATE_0 = 0x4, + I2C_OVER_AUX_WRITE_MOT_1 = 0x5, + I2C_OVER_AUX_READ_MOT_1 = 0x6, + I2C_OVER_AUX_WRITE_STATUS_UPDATE_1 = 0x7, + NATIVE_AUX_WRITE = 0x8, + NATIVE_AUX_READ = 0x9, +}; + +enum ml_tx_mode { + MAIN_LINK_OFF = 0x0, + NORMAL_MODE = 0x1, + TPS1 = 0x2, + TPS2 = 0x3, + TPS3 = 0x4, + PRBS7 = 0x5, + HBR2_COMPLIANCE_EYE_PATTERN = 0x6, + SYMBOL_ERR_RATE_MEASUREMENT_PATTERN = 0x7, + CUTSOM_PATTERN = 0x8, + FAST_LINK_TRAINING = 0x9, + SEMI_AUTO_LINK_TRAINING = 0xa, + REDRIVER_SEMI_AUTO_LINK_TRAINING = 0xb, +}; + +enum dpcd_request { + DPCD_READ = 0x0, + DPCD_WRITE = 0x1, +}; + +enum { + EDID_LENGTH = 128, + EDID_I2C_ADDR = 0x50, + EDID_EXTENSION_FLAG = 0x7e, +}; + +/* + * LUT index corresponds to register value and LUT values corresponds + * to dp data rate supported by the bridge in Mbps unit. + */ +static const unsigned int sn65dsi86_bridge_dp_rate_lut[] = { + 0, 1620, 2160, 2430, 2700, 3240, 4320, 5400 +}; + +enum cb_err sn65dsi86_bridge_read_edid(uint8_t bus, uint8_t chip, struct edid *out) +{ + int ret; + u8 edid[EDID_LENGTH * 2]; + int edid_size = EDID_LENGTH; + + /* Send I2C command to claim EDID I2c slave */ + i2c_writeb(bus, chip, SN_I2C_CLAIM_ADDR_EN1, (EDID_I2C_ADDR << 1) | 0x1); + + /* read EDID */ + ret = i2c_read_bytes(bus, EDID_I2C_ADDR, 0x0, edid, EDID_LENGTH); + if (ret != 0) { + printk(BIOS_ERR, "ERROR: Failed to read EDID.\n"); + return CB_ERR; + } + + if (edid[EDID_EXTENSION_FLAG]) { + edid_size += EDID_LENGTH; + ret = i2c_read_bytes(bus, EDID_I2C_ADDR, EDID_LENGTH, + &edid[EDID_LENGTH], EDID_LENGTH); + if (ret != 0) { + printk(BIOS_ERR, "Failed to read EDID ext block.\n"); + return CB_ERR; + } + } + + if (decode_edid(edid, edid_size, out) != EDID_CONFORMANT) { + printk(BIOS_ERR, "ERROR: Failed to decode EDID.\n"); + return CB_ERR; + } + + return CB_SUCCESS; +} + +static void sn65dsi86_bridge_dpcd_request(uint8_t bus, + uint8_t chip, + unsigned int dpcd_reg, + unsigned int len, + enum dpcd_request request, + uint8_t *data) +{ + int i; + uint32_t length; + uint8_t buf; + uint8_t reg; + + while (len) { + length = MIN(len, 16); + + i2c_writeb(bus, chip, SN_AUX_ADDR_19_16_REG, (dpcd_reg >> 16) & 0xF); + i2c_writeb(bus, chip, SN_AUX_ADDR_15_8_REG, (dpcd_reg >> 8) & 0xFF); + i2c_writeb(bus, chip, SN_AUX_ADDR_7_0_REG, (dpcd_reg) & 0xFF); + i2c_writeb(bus, chip, SN_AUX_LENGTH_REG, length); /* size of 1 Byte data */ + if (request == DPCD_WRITE) { + reg = SN_AUX_WDATA_REG_0; + for (i = 0; i < length; i++) + i2c_writeb(bus, chip, reg++, *data++); + + i2c_writeb(bus, chip, + SN_AUX_CMD_REG, AUX_CMD_SEND | (NATIVE_AUX_WRITE << 4)); + } else { + i2c_writeb(bus, chip, + SN_AUX_CMD_REG, AUX_CMD_SEND | (NATIVE_AUX_READ << 4)); + if (!wait_ms(100, + !i2c_readb(bus, chip, SN_AUX_CMD_REG, + &buf) && !(buf & AUX_CMD_SEND))) { + printk(BIOS_ERR, "ERROR: aux command send failed\n"); + } + + reg = SN_AUX_RDATA_REG_0; + for (i = 0; i < length; i++) { + i2c_readb(bus, chip, reg++, &buf); + *data++ = buf; + } + } + + len -= length; + } +} + +static void sn65dsi86_bridge_valid_dp_rates(uint8_t bus, uint8_t chip, bool rate_valid[]) +{ + unsigned int rate_per_200khz; + uint8_t dpcd_val; + int i, j; + + sn65dsi86_bridge_dpcd_request(bus, chip, + DP_BRIDGE_DPCD_REV, 1, DPCD_READ, &dpcd_val); + if (dpcd_val >= DP_BRIDGE_14) { + /* eDP 1.4 devices must provide a custom table */ + uint8_t sink_rates[DP_MAX_SUPPORTED_RATES * 2]; + + sn65dsi86_bridge_dpcd_request(bus, chip, DP_SUPPORTED_LINK_RATES, + sizeof(sink_rates), + DPCD_READ, sink_rates); + for (i = 0; i < ARRAY_SIZE(sink_rates); i++) { + rate_per_200khz = le16_to_cpu(sink_rates[i]); + + if (!rate_per_200khz) + break; + + for (j = 0; + j < ARRAY_SIZE(sn65dsi86_bridge_dp_rate_lut); + j++) { + if (sn65dsi86_bridge_dp_rate_lut[j] * (MHz / KHz) == + rate_per_200khz * 200) + rate_valid[j] = true; + } + } + + for (i = 0; i < ARRAY_SIZE(sn65dsi86_bridge_dp_rate_lut); i++) { + if (rate_valid[i]) + return; + } + + printk(BIOS_ERR, "No matching eDP rates in table; falling back\n"); + } + + /* On older versions best we can do is use DP_MAX_LINK_RATE */ + sn65dsi86_bridge_dpcd_request(bus, chip, + DP_MAX_LINK_RATE, 1, DPCD_READ, &dpcd_val); + + switch (dpcd_val) { + default: + printk(BIOS_ERR, + "Unexpected max rate (%#x); assuming 5.4 GHz\n", + (int)dpcd_val); + /* fall through */ + case DP_LINK_BW_5_4: + rate_valid[7] = 1; + /* fall through */ + case DP_LINK_BW_2_7: + rate_valid[4] = 1; + /* fall through */ + case DP_LINK_BW_1_62: + rate_valid[1] = 1; + break; + } +} + +static void sn65dsi86_bridge_set_dsi_clock_range(uint8_t bus, uint8_t chip, + struct edid *edid, + uint32_t num_of_lanes, uint32_t bpp) +{ + uint64_t pixel_clk_hz; + uint64_t stream_bit_rate_mhz; + uint64_t min_req_dsi_clk; + + pixel_clk_hz = edid->mode.pixel_clock * KHz; + stream_bit_rate_mhz = (pixel_clk_hz * bpp) / MHz; + + /* For TI the clock frequencies are half the bit rates */ + min_req_dsi_clk = stream_bit_rate_mhz / (num_of_lanes * 2); + + /* for each increment in val, frequency increases by 5MHz */ + min_req_dsi_clk = MAX(MIN_DSI_CLK_FREQ_MHZ, + MIN(MAX_DSI_CLK_FREQ_MHZ, min_req_dsi_clk)) / 5; + i2c_writeb(bus, chip, SN_DSIA_CLK_FREQ_REG, min_req_dsi_clk); +} + +static void sn65dsi86_bridge_set_dp_clock_range(uint8_t bus, uint8_t chip, + struct edid *edid, uint32_t num_of_lanes) +{ + uint64_t stream_bit_rate_khz; + bool rate_valid[ARRAY_SIZE(sn65dsi86_bridge_dp_rate_lut)] = { }; + uint64_t dp_rate_mhz; + int dp_rate_idx, i; + + stream_bit_rate_khz = edid->mode.pixel_clock * 18; + + /* Calculate minimum DP data rate, taking 80% as per DP spec */ + dp_rate_mhz = DIV_ROUND_UP(stream_bit_rate_khz * DP_CLK_FUDGE_NUM, + KHz * num_of_lanes * DP_CLK_FUDGE_DEN); + + for (i = 0; i < ARRAY_SIZE(sn65dsi86_bridge_dp_rate_lut) - 1; i++) + if (sn65dsi86_bridge_dp_rate_lut[i] > dp_rate_mhz) + break; + + sn65dsi86_bridge_valid_dp_rates(bus, chip, rate_valid); + + /* Train until we run out of rates */ + for (dp_rate_idx = i; + dp_rate_idx < ARRAY_SIZE(sn65dsi86_bridge_dp_rate_lut); + dp_rate_idx++) + if (rate_valid[dp_rate_idx]) + break; + + if (dp_rate_idx < ARRAY_SIZE(sn65dsi86_bridge_dp_rate_lut)) + i2c_write_field(bus, chip, SN_DATARATE_CONFIG_REG, dp_rate_idx, 8, 5); + else + printk(BIOS_ERR, "ERROR: valid dp rate not found"); +} + +static void sn65dsi86_bridge_set_bridge_active_timing(uint8_t bus, + uint8_t chip, + struct edid *edid) +{ + i2c_writeb(bus, chip, SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG, + BRIDGE_GETLOWERBYTE(edid->mode.ha)); + i2c_writeb(bus, chip, SN_CHA_ACTIVE_LINE_LENGTH_HIGH_REG, + BRIDGE_GETHIGHERBYTE(edid->mode.ha)); + i2c_writeb(bus, chip, SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG, + BRIDGE_GETLOWERBYTE(edid->mode.va)); + i2c_writeb(bus, chip, SN_CHA_VERTICAL_DISPLAY_SIZE_HIGH_REG, + BRIDGE_GETHIGHERBYTE(edid->mode.va)); + i2c_writeb(bus, chip, SN_CHA_HSYNC_PULSE_WIDTH_LOW_REG, + BRIDGE_GETLOWERBYTE(edid->mode.hspw)); + i2c_writeb(bus, chip, SN_CHA_HSYNC_PULSE_WIDTH_HIGH_REG, + BRIDGE_GETHIGHERBYTE(edid->mode.hspw)); + i2c_writeb(bus, chip, SN_CHA_VSYNC_PULSE_WIDTH_LOW_REG, + BRIDGE_GETLOWERBYTE(edid->mode.vspw)); + i2c_writeb(bus, chip, SN_CHA_VSYNC_PULSE_WIDTH_HIGH_REG, + BRIDGE_GETHIGHERBYTE(edid->mode.vspw)); + i2c_writeb(bus, chip, SN_CHA_HORIZONTAL_BACK_PORCH_REG, + edid->mode.hbl - edid->mode.hso - edid->mode.hspw); + i2c_writeb(bus, chip, SN_CHA_VERTICAL_BACK_PORCH_REG, + edid->mode.vbl - edid->mode.vso - edid->mode.vspw); + i2c_writeb(bus, chip, SN_CHA_HORIZONTAL_FRONT_PORCH_REG, + edid->mode.hso); + i2c_writeb(bus, chip, SN_CHA_VERTICAL_FRONT_PORCH_REG, + edid->mode.vso); +} + +static void sn65dsi86_bridge_link_training(uint8_t bus, uint8_t chip) +{ + uint8_t buf; + + /* enable pll lock */ + i2c_writeb(bus, chip, SN_PLL_ENABLE_REG, 0x1); + + if (!wait_ms(500, + !(i2c_readb(bus, chip, SN_DPPLL_SRC_REG, &buf)) && + (buf & BIT(7)))) { + printk(BIOS_ERR, "ERROR: PLL lock failure\n"); + } + + /* + * The SN65DSI86 only supports ASSR Display Authentication method and + * this method is enabled by default. An eDP panel must support this + * authentication method. We need to enable this method in the eDP panel + * at DisplayPort address 0x0010A prior to link training. + */ + buf = 0x1; + sn65dsi86_bridge_dpcd_request(bus, chip, + DP_BRIDGE_CONFIGURATION_SET, 1, DPCD_WRITE, &buf); + + /* semi auto link training mode */ + i2c_writeb(bus, chip, SN_ML_TX_MODE_REG, 0xa); + + if (!wait_ms(500, + !(i2c_readb(bus, chip, SN_ML_TX_MODE_REG, &buf)) && + (buf & NORMAL_MODE))) { + printk(BIOS_ERR, "ERROR: Link training failed"); + } + +} + +static enum cb_err sn65dsi86_bridge_get_plug_in_status(uint8_t bus, uint8_t chip) +{ + int val; + uint8_t buf; + + val = i2c_readb(bus, chip, SN_HPD_DISABLE_REG, &buf); + if (val == 0 && (buf & HPD_DISABLE)) + return CB_SUCCESS; + + return CB_ERR; +} + +/* + * support bridge HPD function some hardware versions do not support bridge hdp, + * we use 360ms to try to get the hpd single now, if we can not get bridge hpd single, + * it will delay 360ms, also meet the bridge power timing request, to be compatible + * all of the hardware versions + */ +static void sn65dsi86_bridge_wait_hpd(uint8_t bus, uint8_t chip) +{ + if (wait_ms(400, sn65dsi86_bridge_get_plug_in_status(bus, chip))) + return; + + printk(BIOS_WARNING, "HPD detection failed, force hpd\n"); + + /* Force HPD */ + i2c_write_field(bus, chip, SN_HPD_DISABLE_REG, HPD_DISABLE, 1, 0); +} + +static void sn65dsi86_bridge_assr_config(uint8_t bus, uint8_t chip, int enable) +{ + if (enable) + i2c_write_field(bus, chip, SN_ENH_FRAME_REG, VSTREAM_ENABLE, 1, 3); + else + i2c_write_field(bus, chip, SN_ENH_FRAME_REG, VSTREAM_DISABLE, 1, 3); +} + +static int sn65dsi86_bridge_dp_lane_config(uint8_t bus, uint8_t chip) +{ + uint8_t lane_count; + + sn65dsi86_bridge_dpcd_request(bus, chip, DP_MAX_LANE_COUNT, 1, DPCD_READ, &lane_count); + lane_count &= DP_LANE_COUNT_MASK; + i2c_write_field(bus, chip, SN_SSC_CONFIG_REG, MIN(lane_count, 3), 3, 4); + + return lane_count; +} + +void sn65dsi86_bridge_init(uint8_t bus, uint8_t chip, enum dp_pll_clk_src ref_clk) +{ + sn65dsi86_bridge_wait_hpd(bus, chip); + + /* set refclk to 19.2 MHZ */ + i2c_write_field(bus, chip, SN_DPPLL_SRC_REG, ref_clk, 7, 1); +} + +void sn65dsi86_bridge_configure(uint8_t bus, uint8_t chip, + struct edid *edid, uint32_t num_of_lanes, + uint32_t dsi_bpp) +{ + int dp_lanes; + + /* DSI Lanes config */ + i2c_write_field(bus, chip, SN_DSI_LANES_REG, (4 - num_of_lanes), 3, 3); + + /* DP Lane config */ + dp_lanes = sn65dsi86_bridge_dp_lane_config(bus, chip); + + sn65dsi86_bridge_set_dsi_clock_range(bus, chip, edid, num_of_lanes, dsi_bpp); + + sn65dsi86_bridge_set_dp_clock_range(bus, chip, edid, dp_lanes); + + /* Disable vstream */ + sn65dsi86_bridge_assr_config(bus, chip, 0); + sn65dsi86_bridge_link_training(bus, chip); + sn65dsi86_bridge_set_bridge_active_timing(bus, chip, edid); + + /* DP BPP config */ + i2c_writeb(bus, chip, SN_DATA_FORMAT_REG, 0x1); + + /* color bar disabled */ + i2c_writeb(bus, chip, SN_COLOR_BAR_REG, 0x5); + + /* Enable vstream */ + sn65dsi86_bridge_assr_config(bus, chip, 1); +} diff --git a/src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.h b/src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.h new file mode 100644 index 0000000..83b940b --- /dev/null +++ b/src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.h @@ -0,0 +1,22 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef __TI_SN65DSI86BRIDGE_H +#define __TI_SN65DSI86BRIDGE_H + +#include <edid.h> + +enum dp_pll_clk_src { + SN65_SEL_12MHZ = 0x0, + SN65_SEL_19MHZ = 0x1, + SN65_SEL_26MHZ = 0x2, + SN65_SEL_27MHZ = 0x3, + SN65_SEL_38MHZ = 0x4, +}; + +void sn65dsi86_bridge_init(uint8_t bus, uint8_t chip, enum dp_pll_clk_src ref_clk); +void sn65dsi86_bridge_configure(uint8_t bus, uint8_t chip, + struct edid *edid, uint32_t num_of_lines, + uint32_t dsi_bpp); +enum cb_err sn65dsi86_bridge_read_edid(uint8_t bus, uint8_t chip, struct edid *out); + +#endif diff --git a/src/mainboard/google/trogdor/Kconfig b/src/mainboard/google/trogdor/Kconfig index 6a0f912..e0f071c 100644 --- a/src/mainboard/google/trogdor/Kconfig +++ b/src/mainboard/google/trogdor/Kconfig @@ -17,6 +17,7 @@ select EC_GOOGLE_CHROMEEC_SPI if !BOARD_GOOGLE_BUBS select RTC if !BOARD_GOOGLE_BUBS select MISSING_BOARD_RESET if BOARD_GOOGLE_BUBS + select DRIVERS_TI_SN65DSI86BRIDGE select SOC_QUALCOMM_SC7180 select SPI_FLASH select SPI_FLASH_WINBOND
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42899 )
Change subject: sc7180: Add support for sn65dsi86 bridge ......................................................................
Patch Set 20:
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/18563 "QEMU x86 q35/ich9" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/18562 "QEMU x86 i440fx/piix4" (x86_64) using payload SeaBIOS : FAIL : https://lava.9esec.io/r/18561 "QEMU x86 i440fx/piix4" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/18560 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/18559 "HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/18566 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/18565 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/18564
Please note: This test is under development and might not be accurate at all!