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@xxxxxxxxxxxxxx>
> >>> ---
> >>>
> >> 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
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
