Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39612 )
Change subject: sc7180: clock: Add display external clock in coreboot ......................................................................
Patch Set 22:
(5 comments)
https://review.coreboot.org/c/coreboot/+/39612/22/src/soc/qualcomm/sc7180/cl... File src/soc/qualcomm/sc7180/clock.c:
https://review.coreboot.org/c/coreboot/+/39612/22/src/soc/qualcomm/sc7180/cl... PS22, Line 13: #define HALF_DIVIDER(div2x) (div2x ? (div2x - 1) : 0) I'm unclear what this macro is doing, it might be better to do this operation explicitly in mdss_clock_configure() and put a comment next to it to explain it.
https://review.coreboot.org/c/coreboot/+/39612/22/src/soc/qualcomm/sc7180/cl... PS22, Line 109: struct mdss_clock_config mdss_extclk_config[] = { Please do not bind information with string comparisons. Just do something like this:
enum mdss_clock { MDSS_CLK_ESC0 = 0, MDSS_CLK_PCLK0, MDSS_CLK_BYTE0, MDSS_CLK_BYTE0_INTF,
MDSS_CLK_COUNT };
struct sc7180_mnd_clock *mdss_clock[MDSS_CLK_COUNT] = { [MDSS_CLK_ESC0] = &mdss->esc0, ... };
struct u32 *mdss_cbcr[MDSS_CLK_COUNT] = { [MDSS_CLK_ESC0] = &mdss->esc0_cbcr, ... };
...and then below you can do stuff like...
if (clk >= MDSS_CLK_COUNT) return -1;
clock_enable(mdss_cbcr[clk]);
https://review.coreboot.org/c/coreboot/+/39612/22/src/soc/qualcomm/sc7180/cl... PS22, Line 214: ; While we're starting to use these functions for non-boot-critical clocks, it would be a good idea to add timeouts to these btw. You can just write this as
if (!wait_ms(clock_is_off(cbcr_addr), 1)) return -1;
with a reasonable maximum timeout (this would be 1 millisecond, you can make it bigger if necessary). Same goes for clock_enable_vote().
https://review.coreboot.org/c/coreboot/+/39612/22/src/soc/qualcomm/sc7180/in... File src/soc/qualcomm/sc7180/include/soc/clock.h:
https://review.coreboot.org/c/coreboot/+/39612/22/src/soc/qualcomm/sc7180/in... PS22, Line 38: u32 cbcr; Please fit what you're doing into the existing framework rather than rebuilding a copy next to it. The clocks you're adding here are basically sc7180_mnd_clocks and the existing functions like clock_configure seem to fit exactly what you're trying to do, so please use them.
Your one problem is that your clocks don't have the cbcr register right in front of the rest -- fine, then let's change that. The cbcr field here isn't used much anyway, so just take it out of this struct and put it explicitly in front of the struct for the clocks that need it (e.g. sc7180_qupv3_clock and the qup_wrap and qspi stuff in sc7180_gcc).
https://review.coreboot.org/c/coreboot/+/39612/22/src/soc/qualcomm/sc7180/in... PS22, Line 235: struct mdss_clock_config { Please remove this too, I don't know how it got in here.