Attention is currently required from: Ravi kumar, Shelley Chen, mturney mturney. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52662 )
Change subject: sc7180: Add display support for mipi panels ......................................................................
Patch Set 13:
(32 comments)
Patchset:
PS13: Apologies for the long review delay, had a very busy couple of weeks.
Please separate this patch out from the sc7280 patch train and upload it separately, it's not really related to the rest.
File src/mainboard/google/trogdor/Kconfig:
https://review.coreboot.org/c/coreboot/+/52662/comment/3763da62_88b5d4c4 PS13, Line 12: default y if BOARD_GOOGLE_HOMESTAR Please keep the `default n` line here for consistency (even though it's also implicit).
File src/mainboard/google/trogdor/mainboard.c:
https://review.coreboot.org/c/coreboot/+/52662/comment/ad7e632a_0ef9c6dc PS13, Line 81: lanes This needs to be pinfo->lanes, right?
https://review.coreboot.org/c/coreboot/+/52662/comment/880a2caa_f6bc1c97 PS13, Line 86: } else When one side of the if-else uses curly braces, the other one must as well (even for a single statement).
File src/soc/qualcomm/sc7180/display/dsi.c:
https://review.coreboot.org/c/coreboot/+/52662/comment/d6cf9f94_5a08c20c PS13, Line 117: mdss_dsi_clock_config(); Can you remove this now that you're already doing it earlier?
https://review.coreboot.org/c/coreboot/+/52662/comment/62047f23_d47280b1 PS13, Line 163: DSI_PCLK_ON I assume all these changes are safe for eDP? Did you test this on existing eDP devices?
https://review.coreboot.org/c/coreboot/+/52662/comment/85e7fd26_099a6646 PS13, Line 175: setbits32(&dsi0->int_ctrl, DSI_CMD_MODE_DMA_DONE_MASK); Same as below for the clear, can't these all just be written at once?
https://review.coreboot.org/c/coreboot/+/52662/comment/8e8f8111_fa6c4727 PS13, Line 201: 0x00000001 Just write 0x1
https://review.coreboot.org/c/coreboot/+/52662/comment/d3b1354f_aa26ce55 PS13, Line 223: int count) No need for the new line?
https://review.coreboot.org/c/coreboot/+/52662/comment/0031112e_6626eccb PS13, Line 226: uint8_t pload[256]; Are we sure that it's a good idea to DMA straight out of a stack buffer here? What about caching? You're not doing any flushes or anything -- surely this DMA engine isn't cache-snooping when all the others on this system aren't?
We already have an 8K _dma_coherent buffer for the QSPI that is mapped uncached, we should probably just reuse that for this (we are single threaded so we'll never run MIPI init and QSPI code at the same time).
https://review.coreboot.org/c/coreboot/+/52662/comment/15d4c6ee_8f8ea7ca PS13, Line 235: /* Align pload at 8 byte boundary */ Just do this:
uint64_t aligned8[256 / sizeof(uint64_t)]; uint8_t *pload = aligned8;
https://review.coreboot.org/c/coreboot/+/52662/comment/da4741f0_73bf4ff4 PS13, Line 248: size = cm->size; Just do
size = ALIGN_UP(cm->size, PAYLOAD_SIZE);
I believe something like PAYLOAD_ALIGN or PAYLOAD_SIZE_ALIGN would be a better name for that?
https://review.coreboot.org/c/coreboot/+/52662/comment/6d60c6c8_e5697a46 PS13, Line 255: off Just use `pload` here and get rid of `off` (after making sure pload is an aligned buffer like I mentioned above).
https://review.coreboot.org/c/coreboot/+/52662/comment/317ffb4f_ee477310 PS13, Line 255: size Please assert(size < ...amount of bytes in pload array...) before doing the memcpy(). (Maybe add a named constant for that 256.)
https://review.coreboot.org/c/coreboot/+/52662/comment/5e83ef84_e82c1fd9 PS13, Line 257: if (use_tpg == false) { Please don't implement code that will never run. Just get rid of this branch and the use_tpg variable, in your code the TPG always seems to be used. (I assume there is some good reason why we have to use it? The non-TPG code path looks a lot simpler...)
https://review.coreboot.org/c/coreboot/+/52662/comment/904a334f_94fcceeb PS13, Line 264: ALIGN Please use ALIGN_UP() (same thing, but clearer what it does).
https://review.coreboot.org/c/coreboot/+/52662/comment/fb13640c_318df323 PS13, Line 266: if (len > DMA_TPG_FIFO_LEN) { This seems like it should be an assert() or something? We should probably notice if a command is too long, not just silently skip it.
https://review.coreboot.org/c/coreboot/+/52662/comment/646b0802_a7a198ed PS13, Line 281: wmb(); Our I/O accessors (e.g. write32()) have implicit memory barriers, you shouldn't need any extra here (counts for all this code).
https://review.coreboot.org/c/coreboot/+/52662/comment/ce62364a_23f0a26b PS13, Line 307: mdelay Let's define the wait delay in microseconds, not milliseconds, so we can have finer control if necessary. Please also rename `wait` to something that clarifies the unit (e.g. `delay_us`).
https://review.coreboot.org/c/coreboot/+/52662/comment/afa02f5b_09b2ee37 PS13, Line 311: cm++; Rather than doing this just use cmds[i] everywhere (one loop variable is easier to keep track of than two).
https://review.coreboot.org/c/coreboot/+/52662/comment/4128931b_16a5402a PS13, Line 319: write32(&dsi0->int_ctrl,0x0);
space required after that ',' (ctx:VxV)
Please fix all of these.
https://review.coreboot.org/c/coreboot/+/52662/comment/4cf8b540_fe6b8481 PS13, Line 331: setbits32(&dsi0->int_ctrl, DSI_BTA_DONE_AK); Do these bits all need to be written individually? Can't you just
setbits32(&dsi0->int_ctrl, DSI_CMD_MODE_DMA_DONE_AK | DSI_CMD_MODE_MDP_DONE_AK | ...);
?
https://review.coreboot.org/c/coreboot/+/52662/comment/1b81bffa_77eeb8b9 PS13, Line 351: mdelay(500); This seems ridiculous -- surely the mode change doesn't take half a second?
File src/soc/qualcomm/sc7180/display/panel_driver.c:
https://review.coreboot.org/c/coreboot/+/52662/comment/1e331457_5f06f13e PS13, Line 9: #define PANEL_RESET_GPIO GPIO(3) This seems unused?
https://review.coreboot.org/c/coreboot/+/52662/comment/e97d9553_9b6fd311 PS13, Line 40: 0x96 Since these are milliseconds, please write them in decimal (not hex) so they're easier to read.
https://review.coreboot.org/c/coreboot/+/52662/comment/95c90c30_b557325d PS13, Line 46: rm nit: these are usually in capitals
https://review.coreboot.org/c/coreboot/+/52662/comment/a1703c4d_9f6df9e5 PS13, Line 47: .framebuffer_bits_per_pixel = 32, This...
https://review.coreboot.org/c/coreboot/+/52662/comment/0e09e5be_d5e370cd PS13, Line 50: .x_resolution = 1080, ...and this...
https://review.coreboot.org/c/coreboot/+/52662/comment/58928ddc_2a0e03c3 PS13, Line 51: .y_resolution = 2248, ...and this...
https://review.coreboot.org/c/coreboot/+/52662/comment/5f10ac9a_14a499d3 PS13, Line 52: .bytes_per_line = 4320, ...and this shouldn't be set here. Instead, you should call edid_set_framebuffer_bits_per_pixel() in get_panel_config() to compute them from the other values.
https://review.coreboot.org/c/coreboot/+/52662/comment/03b0422d_b100baf2 PS13, Line 73: memcpy(pinfo, &panel_info, sizeof(struct panel_data)); Why not just return a pointer to the panel_info?
File src/soc/qualcomm/sc7180/include/soc/display/mipi_dsi.h:
https://review.coreboot.org/c/coreboot/+/52662/comment/e4aa86d4_b3733c7a PS13, Line 19: uint32_t size; nit: more efficient struct packing would be `payload` first, then `size` and `wait`.