Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34771 )
Change subject: soc/mediatek: dsi: Refactor PHY timing calculation ......................................................................
Patch Set 11:
(4 comments)
https://review.coreboot.org/c/coreboot/+/34771/11/src/soc/mediatek/common/ds... File src/soc/mediatek/common/dsi.c:
https://review.coreboot.org/c/coreboot/+/34771/11/src/soc/mediatek/common/ds... PS11, Line 87: phy_timing->da_hs_exit = 2U * phy_timing->lpx; So how do these work out when you override lpx? Either you need to move these calculations below the override() call (which means they can't be overridden individually, like 8173 does for da_hs_exit), or you need to also override them explicitly for 8173.
(FWIW, I'm also fine with changing this stuff if you check that 8173 still looks okay with the new values. I wouldn't be surprised if they just implemented this a little wrong back then.)
https://review.coreboot.org/c/coreboot/+/34771/11/src/soc/mediatek/common/ds... PS11, Line 94: phy_timing->clk_hs_exit = 2U * phy_timing->lpx; Same for this one (lpx-dependent and must be overridden for 8173).
https://review.coreboot.org/c/coreboot/+/34771/11/src/soc/mediatek/common/ds... PS11, Line 105: da_hs_sync Did you intentionally leave this at 0 for the common implementation?
https://review.coreboot.org/c/coreboot/+/34771/11/src/soc/mediatek/common/ds... PS11, Line 248: struct mtk_phy_timing phy_timing; What's the point of passing this struct in? If it's only for passing stuff to the override, can't you allocate it inside mtk_dsi_phy_timing()?