[U-Boot] [RFC 1/1] dm: video: tegra124: incorrect logical condition

[U-Boot] [RFC 1/1] dm: video: tegra124: incorrect logical condition

[Heinrich Schuchardt]

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

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);
 
-- 
2.15.1

[U-Boot] [RFC 1/1] dm: video: tegra124: incorrect logical condition

[Anatolij Gustschin]

Hi all,

On Wed, 31 Jan 2018 01:16:07 +0100
Heinrich Schuchardt xypron.glpk at 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)

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

[U-Boot] [RFC 1/1] dm: video: tegra124: incorrect logical condition

[Heinrich Schuchardt]

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 at 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
>