Attention is currently required from: Doug Anderson. Hello Doug Anderson,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/52959
to review the following change.
Change subject: drivers/sn65dsi86: Switch EDID reading to use "indirect mode" ......................................................................
drivers/sn65dsi86: Switch EDID reading to use "indirect mode"
The SN65DSI86 eDP bridge supports two ways to read the EDID: for now we've been using "direct mode", which works by basically making the bridge I2C device listen to another chip address besides its own and proxy all requests received there directly to the eDP AUX channel. The great part about that mode is that it is super easy and hassle-free to use. The not so great part about it is that it doesn't work: for EDID extensions, the last byte (which happens to contain the checksum) is somehow always read as zero. We presume this is a hardware bug in the bridge part.
The other, much more annoying way is "indirect mode", where each byte transmitted over the AUX channel has to be manually set up in the I2C registers of the bridge, just like we're already doing with DPCD transactions. Thankfully, we can reuse most of the DPCD code for this so it's not a lot of extra code. It's a bit slower but not as much as you'd expect (26ms instead of 18ms on my board), and the difference is not very relevant compared to common total times for display init.
Also, some of the (previously unused) enum definitions for the AUX_CMD mode field of the bridge had just been plain wrong for some reason, and needed to be fixed to make this work.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I65f80193380d3c3841f9f5c26897ed672f45e15a --- M src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c 1 file changed, 101 insertions(+), 75 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/52959/1
diff --git a/src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c b/src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c index ca41cdb..386f4d5 100644 --- a/src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c +++ b/src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c @@ -127,10 +127,10 @@ 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, + I2C_OVER_AUX_WRITE_STATUS_UPDATE_0 = 0x2, + I2C_OVER_AUX_WRITE_MOT_1 = 0x4, + I2C_OVER_AUX_READ_MOT_1 = 0x5, + I2C_OVER_AUX_WRITE_STATUS_UPDATE_1 = 0x6, NATIVE_AUX_WRITE = 0x8, NATIVE_AUX_READ = 0x9, }; @@ -150,9 +150,11 @@ REDRIVER_SEMI_AUTO_LINK_TRAINING = 0xb, };
-enum dpcd_request { - DPCD_READ = 0x0, - DPCD_WRITE = 0x1, +enum aux_request { + DPCD_READ, + DPCD_WRITE, + I2C_RAW_READ, + I2C_RAW_WRITE, };
enum { @@ -169,29 +171,99 @@ 0, 1620, 2160, 2430, 2700, 3240, 4320, 5400 };
-enum cb_err sn65dsi86_bridge_read_edid(uint8_t bus, uint8_t chip, struct edid *out) +static cb_err_t sn65dsi86_bridge_aux_request(uint8_t bus, + uint8_t chip, + unsigned int target_reg, + unsigned int len, + enum aux_request request, + uint8_t *data) { - int ret; + int rv; + int i; + uint32_t length; + uint8_t buf; + uint8_t reg; + + static const uint8_t command[] = { + [DPCD_WRITE] = NATIVE_AUX_WRITE, + [DPCD_READ] = NATIVE_AUX_READ, + [I2C_RAW_WRITE] = I2C_OVER_AUX_WRITE_MOT_1, + [I2C_RAW_READ] = I2C_OVER_AUX_READ_MOT_1, + }; + + while (len) { + length = MIN(len, 16); + + rv = i2c_writeb(bus, chip, SN_AUX_ADDR_19_16_REG, (target_reg >> 16) & 0xF); + rv |= i2c_writeb(bus, chip, SN_AUX_ADDR_15_8_REG, (target_reg >> 8) & 0xFF); + rv |= i2c_writeb(bus, chip, SN_AUX_ADDR_7_0_REG, (target_reg) & 0xFF); + rv |= i2c_writeb(bus, chip, SN_AUX_LENGTH_REG, length); /* size of 1 Byte data */ + rv |= i2c_writeb(bus, chip, SN_AUX_CMD_REG, AUX_CMD_SEND | (command[request] << 4)); + if (rv) { + printk(BIOS_ERR, "ERROR: aux command setup failed\n"); + return CB_ERR; + } + + if (request == DPCD_WRITE || request == I2C_RAW_WRITE) { + reg = SN_AUX_WDATA_REG_0; + for (i = 0; i < length; i++) + if (i2c_writeb(bus, chip, reg++, *data++)) { + printk(BIOS_ERR, "ERROR: aux command write failed\n"); + return CB_ERR; + } + + } else { + 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"); + return CB_ERR; + } + + reg = SN_AUX_RDATA_REG_0; + for (i = 0; i < length; i++) { + if (i2c_readb(bus, chip, reg++, &buf)) { + printk(BIOS_ERR, "ERROR: aux command read failed\n"); + return CB_ERR; + } + *data++ = buf; + } + } + + len -= length; + } + + return CB_SUCCESS; +} + +cb_err_t sn65dsi86_bridge_read_edid(uint8_t bus, uint8_t chip, struct edid *out) +{ + cb_err_t err; 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) { + uint8_t reg_addr = 0; + err = sn65dsi86_bridge_aux_request(bus, chip, EDID_I2C_ADDR, 1, + I2C_RAW_WRITE, ®_addr); + if (!err) + err = sn65dsi86_bridge_aux_request(bus, chip, EDID_I2C_ADDR, EDID_LENGTH, + I2C_RAW_READ, edid); + if (err) { printk(BIOS_ERR, "ERROR: Failed to read EDID.\n"); - return CB_ERR; + return 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) { + reg_addr = EDID_LENGTH; + err = sn65dsi86_bridge_aux_request(bus, chip, EDID_I2C_ADDR, 1, + I2C_RAW_WRITE, ®_addr); + if (!err) + err = sn65dsi86_bridge_aux_request(bus, chip, EDID_I2C_ADDR, + EDID_LENGTH, I2C_RAW_READ, &edid[EDID_LENGTH]); + if (err) { printk(BIOS_ERR, "Failed to read EDID ext block.\n"); - return CB_ERR; + return err; } }
@@ -203,67 +275,21 @@ 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); + sn65dsi86_bridge_aux_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 */ uint16_t sink_rates[DP_MAX_SUPPORTED_RATES] = {0};
- sn65dsi86_bridge_dpcd_request(bus, chip, DP_SUPPORTED_LINK_RATES, - sizeof(sink_rates), - DPCD_READ, (void *)sink_rates); + sn65dsi86_bridge_aux_request(bus, chip, DP_SUPPORTED_LINK_RATES, + sizeof(sink_rates), + DPCD_READ, (void *)sink_rates); for (i = 0; i < ARRAY_SIZE(sink_rates); i++) { rate_per_200khz = le16_to_cpu(sink_rates[i]);
@@ -288,7 +314,7 @@ }
/* 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); + sn65dsi86_bridge_aux_request(bus, chip, DP_MAX_LINK_RATE, 1, DPCD_READ, &dpcd_val);
switch (dpcd_val) { default: @@ -410,8 +436,8 @@ * at DisplayPort address 0x0010A prior to link training. */ buf = 0x1; - sn65dsi86_bridge_dpcd_request(bus, chip, - DP_BRIDGE_CONFIGURATION_SET, 1, DPCD_WRITE, &buf); + sn65dsi86_bridge_aux_request(bus, chip, + DP_BRIDGE_CONFIGURATION_SET, 1, DPCD_WRITE, &buf);
int i; /* Kernel driver suggests to retry this up to 10 times if it fails. */ for (i = 0; i < 10; i++) { @@ -441,7 +467,7 @@ { uint8_t lane_count;
- sn65dsi86_bridge_dpcd_request(bus, chip, DP_MAX_LANE_COUNT, 1, DPCD_READ, &lane_count); + sn65dsi86_bridge_aux_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);