Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39615 )
Change subject: sc7180: Add display hardware pipe line initialization ......................................................................
Patch Set 25:
(17 comments)
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... File src/mainboard/google/trogdor/Kconfig:
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, Line 21: select DRIVERS_TI_SN65DSI86BRIDGE Your patches have to be in the right order, so if this patch depends on the bridge driver, you have to make the bridge driver come earlier in the patch series.
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... File src/mainboard/google/trogdor/mainboard.c:
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, Line 60: gpio_configure(GPIO(12), GPIO_FUNC_GPIO, GPIO_NO_PULL, You should just use gpio_output(GPIO(12), 1) instead of calling both of these manually.
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, Line 68: static void configure_display(int enable) If we're never calling this with enable == 0, we don't need that parameter and the code depending on it.
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, Line 73: gpio_configure(GPIO(14), GPIO_FUNC_GPIO, GPIO_NO_PULL, This is only correct for Trogdor rev0, for later boards (e.g. Trogdor rev1 or any Lazor) we changed this to GPIO(104). Please define this in board.h (like GPIO_H1_AP_INT is also different for REV0 and others there).
In fact, it's probably best to define all of these in board.h, even the ones that are the same everywhere right now could still change later.
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, Line 80: gpio_set(GPIO(106), 1); This one also changed, it was 106 for Trogdor rev0, rev1 and Lazor rev0, but will move to GPIO(30) for all others. We only have a special Kconfig option for Trogdor-rev0 (for other reasons), so you would use board_id() to decide this one. Write it as
#define GPIO_EN_PP3300_DX_EDP ((CONFIG(BOARD_GOOGLE_TROGDOR) && board_id() < 2) || (CONFIG(BOARD_GOOGLE_LAZOR) && board_id() < 1) ? GPIO(106) : GPIO(30))
in board.h.
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, Line 87: uint8_t num_of_lanes, uint8_t bpp I think you can just hardcode these in this function, rather than passing them in from another function in the same file.
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, Line 94: if (ret) nit: just write
if (mdss_dsi_config(...)) return;
no need for a 'ret'.
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, Line 97: ctrl_mode = read32(&dsi0->ctrl); Let's not open-code this in the mainboard file... create a mdss_dsi_set_command_mode(int enable) function or something like that for this.
(I'm a bit confused why this is necessary, too... does the bridge care what the DSI controller is doing during its init function? Can't we just turn the bridge on first, then initialize the DSI?)
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, Line 101: mdelay(500); Why do we need this delay? 500ms is very long.
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, Line 118: 0x2 Please make named constants for BRIDGE_BUS and BRIDGE_CHIP at the top of this file.
https://review.coreboot.org/c/coreboot/+/39615/25/src/mainboard/google/trogd... PS25, Line 123: edid_set_framebuffer_bits_per_pixel(&ed, You don't really need to call this since it is already the default. (However, if you do want to call it, you should hardcode the 32 here rather than just using the parameter that was set by the default again.)
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/sn... File src/soc/qualcomm/common/sn65dsi86bridge.c:
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/sn... PS17, Line 97: mdelay(100);
I dont find any slave address for the DPCD read/write to implement using direct method.
Sorry, I thought this was possible but I guess I didn't quite understand the difference between the I2C AUX channel and Native DPCD. Looks like you're right and this is the only way.
https://review.coreboot.org/c/coreboot/+/39615/25/src/soc/qualcomm/sc7180/Ma... File src/soc/qualcomm/sc7180/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/39615/25/src/soc/qualcomm/sc7180/Ma... PS25, Line 65: ramstage-$(CONFIG_MAINBOARD_DO_NATIVE_VGA_INIT) += display/dsi.c Please put the Makefile lines for each file together with the patch that is adding that file.
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/sc7180/di... File src/soc/qualcomm/sc7180/display.c:
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/sc7180/di... PS17, Line 80: mdelay(200);
this is required for panel coming up. Moved it before edid read call.
Ack. (200ms is a bit long though, does the panel really need that much?)
https://review.coreboot.org/c/coreboot/+/39615/25/src/soc/qualcomm/sc7180/di... File src/soc/qualcomm/sc7180/display/mdss.c:
https://review.coreboot.org/c/coreboot/+/39615/25/src/soc/qualcomm/sc7180/di... PS25, Line 16: int mdp_dsi_cmd_off(void); You should never need to declare prototypes in .c files. If these functions are supposed to be externally accessible, they belong in a .h file. If they're not, they should be 'static'.
https://review.coreboot.org/c/coreboot/+/39615/25/src/soc/qualcomm/sc7180/di... PS25, Line 156: 0x00000003 Where do all these magic numbers come from? This at least needs some comments.
https://review.coreboot.org/c/coreboot/+/39615/25/src/soc/qualcomm/sc7180/di... PS25, Line 159: MDP_VBIF_RT_BASE + 0x568 Don't do this, define a struct overlay for these registers like you have for the others.