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 3 of 3] ......................................................................
Patch Set 19:
(34 comments)
https://review.coreboot.org/c/coreboot/+/39615/17/src/mainboard/google/trogd... File src/mainboard/google/trogdor/mainboard.c:
https://review.coreboot.org/c/coreboot/+/39615/17/src/mainboard/google/trogd... PS17, Line 39: i2c_init(QUPV3_0_SE2, I2C_SPEED_FAST); /* EDP Bridge I2C */ Please move this to a separate function because it's no longer "just" loading QUPs now (e.g. pack this and the stuff you added in mainboard_init() into a configure_display() or something like that).
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/in... File src/soc/qualcomm/common/include/soc/sn65dsi86bridge.h:
PS17: This part is not Qualcomm-specific, the driver should go in src/drivers/ti/sn65dsi86. Please submit everything going in that directory in a separate patch from the SoC and mainboard stuff.
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/in... PS17, Line 2: * This file is part of the coreboot project. coreboot's license policy just changed, these should all just be one line
/* SPDX-License-Identifier: GPL-2.0-only */
now.
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/in... PS17, Line 16: __SN65DSI86_DP_H Header guard should match the name of the file (and ideally the name of the directory too).
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/in... PS17, Line 105: SEL_12MHZ, Please prefix all constants in this header with something to avoid naming clashes, e.g. SN65_SEL_12MHZ or something like that. (You also don't really need to put all this register-specific stuff in the header. If you just define them in the .c file, it's okay if they're not namespaced.)
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/in... PS17, Line 108: SEL_27MHZ, enums where the actual numerical value of the constant is important should always explicitly assign a value to each field and not just rely on them counting up from 0, e.g.:
SEL_12MHZ = 0, SEL_19MHZ = 1, SEL_26MHZ = 2, ...
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/in... PS17, Line 144: int sn65dsi86_bridge_init(struct msm_fb_panel_data *panel); Blank line before #endif.
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 90: reg, 0x1) Shouldn't this be reg++, *data++?
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/sn... PS17, Line 97: mdelay(100); I assume this is why our boot takes sooooo long to get past display init... this is *waaaay* too slow. The datasheet says the AUX_CMD_SEND bit gets cleared when the transaction is done, so you could just poll for that.
However, I think it would be even better if you just used the "direct method" they describe in the data sheet: if I understand this correctly, you can just pick a random I2C chip address (e.g. (CHIP + 1)), write it to I2C_ADDR_CLAIM1, set the I2C_CLAIM1_EN bit, and then any I2C requests you send to that address will automatically get tunneled to the DP AUX channel. Using this here should be much more efficient than the "indirect method" you implemented (because it's only one I2C transfer per actual DPCD transfer, not 7+).
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/sn... PS17, Line 232: You need to check that dp_rate_idx didn't run over the end without finding a valid rate here.
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/sn... PS17, Line 267: uint8_t data; I'm unclear why you have both buf and data here, can't you just use one everywhere?
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/sn... PS17, Line 272: 500000 wait_us(500000, ...) can be written as wait_ms(500, ...).
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/sn... PS17, Line 273: ( Don't really need the outermost parenthesis here.
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/sn... PS17, Line 274: ((buf & BIT(7)))))) I think this needs to be aligned with the line above (not indented further), that's what the bot is complaining about.
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/sn... PS17, Line 293: printk(BIOS_ERR, "Link training failed"); nit: Put "ERROR: " before serious errors to make them more noticeable.
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/sn... PS17, Line 305: i2c_writeb(I2C_BUS, CHIP, SN_HPD_DISABLE_REG, buf); use i2c_write_field()
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/sn... PS17, Line 314: if (buf & HPD_DISABLE) This should be
if (val == 0 && (buf & HPD_DISABLE))
since if val != 0, buf may not contain valid data.
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/sn... PS17, Line 332: stopwatch_init_msecs_expire(&hpd, 360); You can write this whole thing as
if (wait_ms(360, sn65dsi86_bridge_get_plug_in_status())) return;
Also, it would be good to print a warning if HPD was not detected after the timeout.
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/sn... PS17, Line 347: ret = i2c_readb(I2C_BUS, CHIP, SN_ENH_FRAME_REG, &buf); I don't understand why you're reading here? i2c_write_field() already does the read-modify-write.
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/sn... PS17, Line 364: struct msm_fb_panel_data *panel This should be a generic driver so do not use Qualcomm-specific data structures.
I don't really understand why you need to pass hardcoded panel information in from the mainboard here. This MIPI-to-eDP bridge connects to an eDP panel, which (like all eDP panels) should have an EDID structure describing panel parameters that you can read out via the AUX channel. Please see the SN65DSI86 datasheet for information about how to read out the EDID. (They describe both a "direct" method via the I2C_ADDR_CLAIMx registers and an "indirect" method via packet passthrough. Direct is probably more efficient but either of them should work.) After you have the raw EDID data, you can use the decode_edid() function to extract a struct edid out of it which contains all these parameters. Like I mentioned before, this should all work very similar to the src/drivers/parade/ps8640/ps8640.c driver (you can see it used in src/mainboard/google/oak/mainboard.c, for example).
For the num_of_lanes field I would just pass that as a raw 'int' parameter.
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/sn... PS17, Line 384: SEL_19MHZ Since other boards might use another reference clock, this should probably be passed in as a parameter.
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/common/sn... PS17, Line 387: I2C_BUS, CHIP Since this should be a generic driver, we can't hardcode these obviously. Bus number should be passed in as a parameter. Chip address can also be passed in or you could just hardcode it for now (since we're unlikely to ever need the address selection).
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 69: static int sn65dsi86_bridge_read_edid(struct edid *out) Oh here you have the EDID stuff. Why here? This function belongs in the bridge driver, and needs to be called from mainboard code.
The bridge driver and the SoC display code need to be 100% completely independent and not share any headers other than common stuff like struct edid. The mainboard code needs to tie them together -- first initializing the bridge, using it to fetch the EDID, then passing that EDID (and whatever other info may be necessary, e.g. number of MIPI lanes or whatever) to the SoC display initialization code.
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/sc7180/di... PS17, Line 79: 0xA1 This should be written as
(EDID_I2C_ADDR << 1) | 0x1
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/sc7180/di... PS17, Line 80: mdelay(200); Why the wait? The bridge datasheet doesn't say anything about waiting here (especially not 200ms). If this wait is related to the panel coming up, it should not be in bridge code.
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/sc7180/di... PS17, Line 83: ret = i2c_read_bytes(I2C_BUS, EDID_I2C_ADDR, 0x0, edid, EDID_LENGTH); Oh, and here you're already using the "direct" AUX channel access thing. Very good. You should write the DPCD interface like this as well. (You can just write I2C_CLAIM_ADDR_EN1 once in the init function and then keep using it.)
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/sc7180/di... PS17, Line 152: pan_type = init_panel_data(&panelstruct, There should be no concept of a "panel" in the SoC code beyond what struct EDID contains (and maybe a few extra fields that are probably easier to pass manually). And all this information needs to come in from the mainboard. The code under soc/qualcomm/sc7180 should not contain any information about a specific panel from a specific vendor.
Basically, the words "panel" or "oem" should probably not appear anywhere in the code under soc/qualcomm/sc7180. (If there are specific problems with designing it this way, please let me know so we can discuss them in detail. In general, for eDP panels the EDID should always be enough to describe anything the display stack should need to know about the panel. That's how it has worked on all our other platforms before.
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/sc7180/di... PS17, Line 156: if (pan_type == PANEL_TYPE_DSI) { (This is just an example of stuff that seems unnecessary... do we have any kinds of panels other than DSI? Isn't this just a driver for a MIPI DSI controller? Same with the pinfo->type where MIPI_VIDEO_PANEL is the only type we'll presumably ever have... please do not add complication that we'll never have a use for.)
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/sc7180/di... PS17, Line 162: panel.fb.width = edid->x_resolution; You should be passing struct edid (or struct edid_mode) around rather than defining your own structures for all of this.
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/sc7180/in... File src/soc/qualcomm/sc7180/include/soc/display/panel_sn65dsix6_auo_bll6xak01_dsi_video.h:
PS17: As mentioned in the display.c comments, this whole file basically shouldn't exist.
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/sc7180/in... PS17, Line 25: static char slave_panel_node_id[256] = "qcom,dsi_sn65dsix6_auo_b116xak01_video"; None of these things are necessary to put an image on that screen.
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/sc7180/in... PS17, Line 36: 60 You can get this from (struct edid).refresh.
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/sc7180/in... PS17, Line 53: 24 Information about the MIPI connection between panel and bridge can probably just be passed directly as parameters from the mainboard to the display init function. For example, look how this is done in https://review.coreboot.org/cgit/coreboot.git/tree/src/mainboard/google/oak/... -- they just pass the amount of lanes, a video format identifier, and a couple of flags to handle other stuff to the SoC display function, and that's it. You shouldn't really need much more than that either. (If you want I'm okay with packaging this stuff into a structure too, but it should only contain the few fields that you really need, not all this stuff that will always be zeroes in practice. And nothing that's redundant with struct edid.)
https://review.coreboot.org/c/coreboot/+/39615/17/src/soc/qualcomm/sc7180/in... PS17, Line 85: 0x0, 0x11, 0x3, 0x5, 0x1E, 0x1E, 0x4, 0x4, 0x2, 0x3, 0x4 Okay, I'm fully lost at this point... I don't know what any of this is. I've never seen any other eDP panel that needs so much hardcoded stuff, eDP panels should self-report everything needed through the EDID. That's how every other platform I have seen works.
I don't see the kernel having anything like this hardcoded anywhere. If they don't need it to get the display running, why do we?