On Mon, Apr 11, 2022 at 07:00:51PM +0200, Alex Bee wrote:
Am 11.04.22 um 09:53 schrieb Sascha Hauer:
On Sun, Apr 10, 2022 at 01:31:23PM +0200, Alex Bee wrote:
Am 08.04.22 um 13:22 schrieb Sascha Hauer:
The driver checks if the pixel clock of the given mode matches an entry in the mpll config table. The frequencies in the mpll table are meant as a frequency range up to which the entry works, not as a frequency that must match the pixel clock. The downstream Kernel also does not have this check, so drop it to allow for more display resolutions.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de
You're correct: That frequency is meant to be greater or equal. But I'm not sure if it makes sense to completely drop it - what happens for clocks rates > 600 MHz which might be supported by later generation sinks (HDMI 2.1 or later)? As these are not supported by the IPs/PHYs currently supported by that driver a reason a simple
int i; for (i = 0; mpll_cfg[i].mpixelclock != (~0UL); i++) {
if (pclk == mpll_cfg[i].mpixelclock) {
if (pclk >= mpll_cfg[i].mpixelclock) {Should be <=
Sure, sorry 'bout that.
No other user currently in the tree has this check.
I dont't think that's true - it might not be checked against mpll table, but max pixelclocks are checked in meson_encoder_hdmi_mode_valid for amlogic, in sun8i_dw_hdmi_mode_valid_a83t and sun8i_dw_hdmi_mode_valid_h6 for allwinner and rcar_hdmi_mode_valid for rcar platform. There is no other point in rockchip dw-hdmi platform driver where this is currently checked.
hdmi_phy_configure_dwc_hdmi_3d_tx() has this:
/* PLL/MPLL Cfg - always match on final entry */ for (; mpll_config->mpixelclock != ~0UL; mpll_config++) if (mpixelclock <= mpll_config->mpixelclock) break;
for (; curr_ctrl->mpixelclock != ~0UL; curr_ctrl++) if (mpixelclock <= curr_ctrl->mpixelclock) break;
for (; phy_config->mpixelclock != ~0UL; phy_config++) if (mpixelclock <= phy_config->mpixelclock) break;
if (mpll_config->mpixelclock == ~0UL || curr_ctrl->mpixelclock == ~0UL || phy_config->mpixelclock == ~0UL) return -EINVAL;
This means we already have this check and others already in the generic part of the dw-hdmi driver. Should we maybe do the above in dw_hdmi_bridge_mode_valid() instead of doing it in the users?
I guess that's not possible, due to the different kind of phys which are used for dw-hdmi. The checks you are refering to, are only done for dw hdmi tx phys, but not for "vendor phys" (like for RK3328 and RK3228, for example) - those have have own drivers which are handled only in dw-hdmi platform driver. Therefore this check should remain here for Rockchip also.
Ok, agreed. I'll keep the check for the next round and just replace the == with <=.
Sascha