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