On 03/06/2018 11:08 AM, Anatolij Gustschin wrote:

Hi all,

On Wed, 31 Jan 2018 01:16:07 +0100 Heinrich Schuchardt xypron.glpk@gmx.de wrote:

...

2 << 24 | A is always true. To use check against a bitmask we need &.

it is always true, but here we are not checking against a bitmask, so the patch is wrong.

We set or clear register bit (depending on 'is_lvds' value) together with another register bits for ROTCLK config.

So, I think the code should be

2 << CSTM_ROTCLK_SHIFT |
(is_lvds ? CSTM_LVDS_EN_ENABLE : CSTM_LVDS_EN_DISABLE)

Yes, it could be that the author just didn't consider operator precedence in patch 00f37327524.

tegra_dc_sor_enable_lane_sequencer(), tegra_dc_sor_power_up(), and tegra_dc_sor_config_panel() are only called with is_lvds = 0. We might want to eleminate the parameter.

The U-Boot lines in question relate to Linux kernel drivers/gpu/drm/tegra/hdmi.c:

value = tegra_hdmi_readl(hdmi, HDMI_NV_PDISP_SOR_CSTM); value &= ~SOR_CSTM_ROTCLK(~0); value |= SOR_CSTM_ROTCLK(2); value |= SOR_CSTM_PLLDIV; value &= ~SOR_CSTM_LVDS_ENABLE; value &= ~SOR_CSTM_MODE_MASK; value |= SOR_CSTM_MODE_TMDS; tegra_hdmi_writel(hdmi, value, HDMI_NV_PDISP_SOR_CSTM);

The kernel code only uses the disabling of LVDS. And yes it is used in conjunction with the 2 << CSTM_ROTCLK_SHIFT (== SOR_CSTM_ROTCLK(2)) bit.

With the change that you suggested we would flip both bits to the values used by the kernel.

Patch 00f37327524 was about adding support for eDP LCD panels. So probably the author wanted to disable LVDS here.

Regards

Heinrich

But I do not have the hardware to test. Maybe Simon ?

...

Identified with cppcheck.

Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de

I do not have the hardware available. But the current coding is fishy.

Please, clarify what should be coded here.

drivers/video/tegra124/sor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/tegra124/sor.c b/drivers/video/tegra124/sor.c index 700ab25d46..4b3381fae2 100644 --- a/drivers/video/tegra124/sor.c +++ b/drivers/video/tegra124/sor.c @@ -669,7 +669,7 @@ static void tegra_dc_sor_config_panel(struct tegra_dc_sor_data *sor, tegra_sor_write_field(sor, CSTM, CSTM_ROTCLK_DEFAULT_MASK | CSTM_LVDS_EN_ENABLE,

• 	      2 << CSTM_ROTCLK_SHIFT |
  

• 	      2 << CSTM_ROTCLK_SHIFT &
        is_lvds ? CSTM_LVDS_EN_ENABLE :
        CSTM_LVDS_EN_DISABLE);
  
  

Thanks,

Anatolij