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?