diff mbox series

[02/11] drm/bridge: tc358768: Fix bit updates

Message ID 20230804-tc358768-v1-2-1afd44b7826b@ideasonboard.com (mailing list archive)
State New, archived
Headers show
Series drm/bridge: tc358768: Fixes and timings improvements | expand

Commit Message

Tomi Valkeinen Aug. 4, 2023, 10:44 a.m. UTC 
The driver has a few places where it does:

if (thing_is_enabled_in_config)
	update_thing_bit_in_hw()

This means that if the thing is _not_ enabled, the bit never gets
cleared. This affects the h/vsyncs and continuous DSI clock bits.

Fix the driver to always update the bit.

Fixes: ff1ca6397b1d ("drm/bridge: Add tc358768 driver")
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/tc358768.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Comments

Péter Ujfalusi Aug. 11, 2023, 4:23 p.m. UTC | #1 
On 04/08/2023 13:44, Tomi Valkeinen wrote:
> The driver has a few places where it does:
> 
> if (thing_is_enabled_in_config)
> 	update_thing_bit_in_hw()
> 
> This means that if the thing is _not_ enabled, the bit never gets
> cleared. This affects the h/vsyncs and continuous DSI clock bits.

I guess the idea was to keep the reset value unless it needs to be flipped.

> 
> Fix the driver to always update the bit.
> 
> Fixes: ff1ca6397b1d ("drm/bridge: Add tc358768 driver")
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/tc358768.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
> index bc97a837955b..b668f77673c3 100644
> --- a/drivers/gpu/drm/bridge/tc358768.c
> +++ b/drivers/gpu/drm/bridge/tc358768.c
> @@ -794,8 +794,8 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>  		val |= BIT(i + 1);
>  	tc358768_write(priv, TC358768_HSTXVREGEN, val);
>  
> -	if (!(mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS))
> -		tc358768_write(priv, TC358768_TXOPTIONCNTRL, 0x1);
> +	tc358768_write(priv, TC358768_TXOPTIONCNTRL,
> +		       (mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) ? 0 : BIT(0));
>  
>  	/* TXTAGOCNT[26:16] RXTASURECNT[10:0] */
>  	val = tc358768_to_ns((lptxcnt + 1) * dsibclk_nsk * 4);
> @@ -861,11 +861,12 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>  	tc358768_write(priv, TC358768_DSI_HACT, hact);
>  
>  	/* VSYNC polarity */
> -	if (!(mode->flags & DRM_MODE_FLAG_NVSYNC))
> -		tc358768_update_bits(priv, TC358768_CONFCTL, BIT(5), BIT(5));
> +	tc358768_update_bits(priv, TC358768_CONFCTL, BIT(5),
> +			     (mode->flags & DRM_MODE_FLAG_PVSYNC) ? BIT(5) : 0);

Was this the reverse before and should be:
(mode->flags & DRM_MODE_FLAG_PVSYNC) ? 0 : BIT(5)

> +
>  	/* HSYNC polarity */
> -	if (mode->flags & DRM_MODE_FLAG_PHSYNC)
> -		tc358768_update_bits(priv, TC358768_PP_MISC, BIT(0), BIT(0));
> +	tc358768_update_bits(priv, TC358768_PP_MISC, BIT(0),
> +			     (mode->flags & DRM_MODE_FLAG_PHSYNC) ? BIT(0) : 0);
>  
>  	/* Start DSI Tx */
>  	tc358768_write(priv, TC358768_DSI_START, 0x1);
>
Tomi Valkeinen Aug. 11, 2023, 5:02 p.m. UTC | #2 
On 11/08/2023 19:23, Péter Ujfalusi wrote:
> 
> 
> On 04/08/2023 13:44, Tomi Valkeinen wrote:
>> The driver has a few places where it does:
>>
>> if (thing_is_enabled_in_config)
>> 	update_thing_bit_in_hw()
>>
>> This means that if the thing is _not_ enabled, the bit never gets
>> cleared. This affects the h/vsyncs and continuous DSI clock bits.
> 
> I guess the idea was to keep the reset value unless it needs to be flipped.
> 
>>
>> Fix the driver to always update the bit.
>>
>> Fixes: ff1ca6397b1d ("drm/bridge: Add tc358768 driver")
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/gpu/drm/bridge/tc358768.c | 13 +++++++------
>>   1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
>> index bc97a837955b..b668f77673c3 100644
>> --- a/drivers/gpu/drm/bridge/tc358768.c
>> +++ b/drivers/gpu/drm/bridge/tc358768.c
>> @@ -794,8 +794,8 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>>   		val |= BIT(i + 1);
>>   	tc358768_write(priv, TC358768_HSTXVREGEN, val);
>>   
>> -	if (!(mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS))
>> -		tc358768_write(priv, TC358768_TXOPTIONCNTRL, 0x1);
>> +	tc358768_write(priv, TC358768_TXOPTIONCNTRL,
>> +		       (mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) ? 0 : BIT(0));
>>   
>>   	/* TXTAGOCNT[26:16] RXTASURECNT[10:0] */
>>   	val = tc358768_to_ns((lptxcnt + 1) * dsibclk_nsk * 4);
>> @@ -861,11 +861,12 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>>   	tc358768_write(priv, TC358768_DSI_HACT, hact);
>>   
>>   	/* VSYNC polarity */
>> -	if (!(mode->flags & DRM_MODE_FLAG_NVSYNC))
>> -		tc358768_update_bits(priv, TC358768_CONFCTL, BIT(5), BIT(5));
>> +	tc358768_update_bits(priv, TC358768_CONFCTL, BIT(5),
>> +			     (mode->flags & DRM_MODE_FLAG_PVSYNC) ? BIT(5) : 0);
> 
> Was this the reverse before and should be:
> (mode->flags & DRM_MODE_FLAG_PVSYNC) ? 0 : BIT(5)

Bit 5 is 1 for active high vsync polarity. The test was previously 
!nvsync, i.e. the same as pvsync.

  Tomi
Maxim Schwalm Aug. 13, 2023, 12:23 a.m. UTC | #3 
Hi,

On 11.08.23 19:02, Tomi Valkeinen wrote:
> On 11/08/2023 19:23, Péter Ujfalusi wrote:
>>
>>
>> On 04/08/2023 13:44, Tomi Valkeinen wrote:
>>> The driver has a few places where it does:
>>>
>>> if (thing_is_enabled_in_config)
>>> 	update_thing_bit_in_hw()
>>>
>>> This means that if the thing is _not_ enabled, the bit never gets
>>> cleared. This affects the h/vsyncs and continuous DSI clock bits.
>>
>> I guess the idea was to keep the reset value unless it needs to be flipped.
>>
>>>
>>> Fix the driver to always update the bit.
>>>
>>> Fixes: ff1ca6397b1d ("drm/bridge: Add tc358768 driver")
>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>> ---
>>>   drivers/gpu/drm/bridge/tc358768.c | 13 +++++++------
>>>   1 file changed, 7 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
>>> index bc97a837955b..b668f77673c3 100644
>>> --- a/drivers/gpu/drm/bridge/tc358768.c
>>> +++ b/drivers/gpu/drm/bridge/tc358768.c
>>> @@ -794,8 +794,8 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>>>   		val |= BIT(i + 1);
>>>   	tc358768_write(priv, TC358768_HSTXVREGEN, val);
>>>   
>>> -	if (!(mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS))
>>> -		tc358768_write(priv, TC358768_TXOPTIONCNTRL, 0x1);
>>> +	tc358768_write(priv, TC358768_TXOPTIONCNTRL,
>>> +		       (mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) ? 0 : BIT(0));
>>>   
>>>   	/* TXTAGOCNT[26:16] RXTASURECNT[10:0] */
>>>   	val = tc358768_to_ns((lptxcnt + 1) * dsibclk_nsk * 4);
>>> @@ -861,11 +861,12 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>>>   	tc358768_write(priv, TC358768_DSI_HACT, hact);
>>>   
>>>   	/* VSYNC polarity */
>>> -	if (!(mode->flags & DRM_MODE_FLAG_NVSYNC))
>>> -		tc358768_update_bits(priv, TC358768_CONFCTL, BIT(5), BIT(5));
>>> +	tc358768_update_bits(priv, TC358768_CONFCTL, BIT(5),
>>> +			     (mode->flags & DRM_MODE_FLAG_PVSYNC) ? BIT(5) : 0);
>>
>> Was this the reverse before and should be:
>> (mode->flags & DRM_MODE_FLAG_PVSYNC) ? 0 : BIT(5)
> 
> Bit 5 is 1 for active high vsync polarity. The test was previously 
> !nvsync, i.e. the same as pvsync.

this statement doesn't seem to be true, since this change causes a
regression on the Asus TF700T. Apparently, !nvsync is true and pvsync is
false in the present case. 

Best regards,
Maxim
Tomi Valkeinen Aug. 14, 2023, 6:34 a.m. UTC | #4 
On 13/08/2023 03:23, Maxim Schwalm wrote:
> Hi,
> 
> On 11.08.23 19:02, Tomi Valkeinen wrote:
>> On 11/08/2023 19:23, Péter Ujfalusi wrote:
>>>
>>>
>>> On 04/08/2023 13:44, Tomi Valkeinen wrote:
>>>> The driver has a few places where it does:
>>>>
>>>> if (thing_is_enabled_in_config)
>>>> 	update_thing_bit_in_hw()
>>>>
>>>> This means that if the thing is _not_ enabled, the bit never gets
>>>> cleared. This affects the h/vsyncs and continuous DSI clock bits.
>>>
>>> I guess the idea was to keep the reset value unless it needs to be flipped.
>>>
>>>>
>>>> Fix the driver to always update the bit.
>>>>
>>>> Fixes: ff1ca6397b1d ("drm/bridge: Add tc358768 driver")
>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>> ---
>>>>    drivers/gpu/drm/bridge/tc358768.c | 13 +++++++------
>>>>    1 file changed, 7 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
>>>> index bc97a837955b..b668f77673c3 100644
>>>> --- a/drivers/gpu/drm/bridge/tc358768.c
>>>> +++ b/drivers/gpu/drm/bridge/tc358768.c
>>>> @@ -794,8 +794,8 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>>>>    		val |= BIT(i + 1);
>>>>    	tc358768_write(priv, TC358768_HSTXVREGEN, val);
>>>>    
>>>> -	if (!(mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS))
>>>> -		tc358768_write(priv, TC358768_TXOPTIONCNTRL, 0x1);
>>>> +	tc358768_write(priv, TC358768_TXOPTIONCNTRL,
>>>> +		       (mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) ? 0 : BIT(0));
>>>>    
>>>>    	/* TXTAGOCNT[26:16] RXTASURECNT[10:0] */
>>>>    	val = tc358768_to_ns((lptxcnt + 1) * dsibclk_nsk * 4);
>>>> @@ -861,11 +861,12 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>>>>    	tc358768_write(priv, TC358768_DSI_HACT, hact);
>>>>    
>>>>    	/* VSYNC polarity */
>>>> -	if (!(mode->flags & DRM_MODE_FLAG_NVSYNC))
>>>> -		tc358768_update_bits(priv, TC358768_CONFCTL, BIT(5), BIT(5));
>>>> +	tc358768_update_bits(priv, TC358768_CONFCTL, BIT(5),
>>>> +			     (mode->flags & DRM_MODE_FLAG_PVSYNC) ? BIT(5) : 0);
>>>
>>> Was this the reverse before and should be:
>>> (mode->flags & DRM_MODE_FLAG_PVSYNC) ? 0 : BIT(5)
>>
>> Bit 5 is 1 for active high vsync polarity. The test was previously
>> !nvsync, i.e. the same as pvsync.
> 
> this statement doesn't seem to be true, since this change causes a
> regression on the Asus TF700T. Apparently, !nvsync is true and pvsync is
> false in the present case.

panasonic_vvx10f004b00_mode in panel_simple.c doesn't seem to have mode 
flags set. I would say that means the panel doesn't care about the sync 
polarities (which obviously is not the case), but maybe there's an 
assumption that if sync polarities are not set, the default is... 
positive? But I can't find any mention about this.

Does it work for you if you set the polarities in 
panasonic_vvx10f004b00_mode?

  Tomi
Maxim Schwalm Aug. 15, 2023, 5:21 p.m. UTC | #5 
On 14.08.23 08:34, Tomi Valkeinen wrote:
> On 13/08/2023 03:23, Maxim Schwalm wrote:
>> Hi,
>>
>> On 11.08.23 19:02, Tomi Valkeinen wrote:
>>> On 11/08/2023 19:23, Péter Ujfalusi wrote:
>>>>
>>>>
>>>> On 04/08/2023 13:44, Tomi Valkeinen wrote:
>>>>> The driver has a few places where it does:
>>>>>
>>>>> if (thing_is_enabled_in_config)
>>>>> 	update_thing_bit_in_hw()
>>>>>
>>>>> This means that if the thing is _not_ enabled, the bit never gets
>>>>> cleared. This affects the h/vsyncs and continuous DSI clock bits.
>>>>
>>>> I guess the idea was to keep the reset value unless it needs to be flipped.
>>>>
>>>>>
>>>>> Fix the driver to always update the bit.
>>>>>
>>>>> Fixes: ff1ca6397b1d ("drm/bridge: Add tc358768 driver")
>>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>>> ---
>>>>>    drivers/gpu/drm/bridge/tc358768.c | 13 +++++++------
>>>>>    1 file changed, 7 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
>>>>> index bc97a837955b..b668f77673c3 100644
>>>>> --- a/drivers/gpu/drm/bridge/tc358768.c
>>>>> +++ b/drivers/gpu/drm/bridge/tc358768.c
>>>>> @@ -794,8 +794,8 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>>>>>    		val |= BIT(i + 1);
>>>>>    	tc358768_write(priv, TC358768_HSTXVREGEN, val);
>>>>>    
>>>>> -	if (!(mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS))
>>>>> -		tc358768_write(priv, TC358768_TXOPTIONCNTRL, 0x1);
>>>>> +	tc358768_write(priv, TC358768_TXOPTIONCNTRL,
>>>>> +		       (mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) ? 0 : BIT(0));
>>>>>    
>>>>>    	/* TXTAGOCNT[26:16] RXTASURECNT[10:0] */
>>>>>    	val = tc358768_to_ns((lptxcnt + 1) * dsibclk_nsk * 4);
>>>>> @@ -861,11 +861,12 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>>>>>    	tc358768_write(priv, TC358768_DSI_HACT, hact);
>>>>>    
>>>>>    	/* VSYNC polarity */
>>>>> -	if (!(mode->flags & DRM_MODE_FLAG_NVSYNC))
>>>>> -		tc358768_update_bits(priv, TC358768_CONFCTL, BIT(5), BIT(5));
>>>>> +	tc358768_update_bits(priv, TC358768_CONFCTL, BIT(5),
>>>>> +			     (mode->flags & DRM_MODE_FLAG_PVSYNC) ? BIT(5) : 0);
>>>>
>>>> Was this the reverse before and should be:
>>>> (mode->flags & DRM_MODE_FLAG_PVSYNC) ? 0 : BIT(5)
>>>
>>> Bit 5 is 1 for active high vsync polarity. The test was previously
>>> !nvsync, i.e. the same as pvsync.
>>
>> this statement doesn't seem to be true, since this change causes a
>> regression on the Asus TF700T. Apparently, !nvsync is true and pvsync is
>> false in the present case.
> 
> panasonic_vvx10f004b00_mode in panel_simple.c doesn't seem to have mode 
> flags set. I would say that means the panel doesn't care about the sync 
> polarities (which obviously is not the case), but maybe there's an 
> assumption that if sync polarities are not set, the default is... 
> positive? But I can't find any mention about this.
> 
> Does it work for you if you set the polarities in 
> panasonic_vvx10f004b00_mode?

The panel seems to work with either negative or positive H-/Vsync in
conjunction with the attached patch from Thierry. Currently, the display
controller is unconditionally programmed for positive H-/Vsync though.
What should be done in this case?

BTW, the vendor kernel configures the display controller as well as the
bridge for negative H-/Vsync.

Best regards,
Maxim
Tomi Valkeinen Aug. 16, 2023, 8:14 a.m. UTC | #6 
On 15/08/2023 20:21, Maxim Schwalm wrote:
> On 14.08.23 08:34, Tomi Valkeinen wrote:
>> On 13/08/2023 03:23, Maxim Schwalm wrote:
>>> Hi,
>>>
>>> On 11.08.23 19:02, Tomi Valkeinen wrote:
>>>> On 11/08/2023 19:23, Péter Ujfalusi wrote:
>>>>>
>>>>>
>>>>> On 04/08/2023 13:44, Tomi Valkeinen wrote:
>>>>>> The driver has a few places where it does:
>>>>>>
>>>>>> if (thing_is_enabled_in_config)
>>>>>> 	update_thing_bit_in_hw()
>>>>>>
>>>>>> This means that if the thing is _not_ enabled, the bit never gets
>>>>>> cleared. This affects the h/vsyncs and continuous DSI clock bits.
>>>>>
>>>>> I guess the idea was to keep the reset value unless it needs to be flipped.
>>>>>
>>>>>>
>>>>>> Fix the driver to always update the bit.
>>>>>>
>>>>>> Fixes: ff1ca6397b1d ("drm/bridge: Add tc358768 driver")
>>>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/bridge/tc358768.c | 13 +++++++------
>>>>>>     1 file changed, 7 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
>>>>>> index bc97a837955b..b668f77673c3 100644
>>>>>> --- a/drivers/gpu/drm/bridge/tc358768.c
>>>>>> +++ b/drivers/gpu/drm/bridge/tc358768.c
>>>>>> @@ -794,8 +794,8 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>>>>>>     		val |= BIT(i + 1);
>>>>>>     	tc358768_write(priv, TC358768_HSTXVREGEN, val);
>>>>>>     
>>>>>> -	if (!(mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS))
>>>>>> -		tc358768_write(priv, TC358768_TXOPTIONCNTRL, 0x1);
>>>>>> +	tc358768_write(priv, TC358768_TXOPTIONCNTRL,
>>>>>> +		       (mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) ? 0 : BIT(0));
>>>>>>     
>>>>>>     	/* TXTAGOCNT[26:16] RXTASURECNT[10:0] */
>>>>>>     	val = tc358768_to_ns((lptxcnt + 1) * dsibclk_nsk * 4);
>>>>>> @@ -861,11 +861,12 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>>>>>>     	tc358768_write(priv, TC358768_DSI_HACT, hact);
>>>>>>     
>>>>>>     	/* VSYNC polarity */
>>>>>> -	if (!(mode->flags & DRM_MODE_FLAG_NVSYNC))
>>>>>> -		tc358768_update_bits(priv, TC358768_CONFCTL, BIT(5), BIT(5));
>>>>>> +	tc358768_update_bits(priv, TC358768_CONFCTL, BIT(5),
>>>>>> +			     (mode->flags & DRM_MODE_FLAG_PVSYNC) ? BIT(5) : 0);
>>>>>
>>>>> Was this the reverse before and should be:
>>>>> (mode->flags & DRM_MODE_FLAG_PVSYNC) ? 0 : BIT(5)
>>>>
>>>> Bit 5 is 1 for active high vsync polarity. The test was previously
>>>> !nvsync, i.e. the same as pvsync.
>>>
>>> this statement doesn't seem to be true, since this change causes a
>>> regression on the Asus TF700T. Apparently, !nvsync is true and pvsync is
>>> false in the present case.
>>
>> panasonic_vvx10f004b00_mode in panel_simple.c doesn't seem to have mode
>> flags set. I would say that means the panel doesn't care about the sync
>> polarities (which obviously is not the case), but maybe there's an
>> assumption that if sync polarities are not set, the default is...
>> positive? But I can't find any mention about this.
>>
>> Does it work for you if you set the polarities in
>> panasonic_vvx10f004b00_mode?
> 
> The panel seems to work with either negative or positive H-/Vsync in
> conjunction with the attached patch from Thierry. Currently, the display
> controller is unconditionally programmed for positive H-/Vsync though.
> What should be done in this case?
> 
> BTW, the vendor kernel configures the display controller as well as the
> bridge for negative H-/Vsync.

Ah, of course, I wasn't thinking. It's a DSI panel (obviously...), so it 
doesn't have sync polarities and as such it doesn't really make sense to 
define them in the panel-simple.c.

But we still need an agreed sync polarity between the tegra and the 
tc358768, as that is a parallel video bus. And that polarity is not 
defined anywhere, as it is expected to come from the panel.

Maybe tc358768 should have a mode-fixup, where it sets the polarities if 
they are not defined? I'll have to look at this a bit more.

  Tomi
Tomi Valkeinen Aug. 16, 2023, 8:21 a.m. UTC | #7 
On 15/08/2023 20:21, Maxim Schwalm wrote:
> On 14.08.23 08:34, Tomi Valkeinen wrote:
>> On 13/08/2023 03:23, Maxim Schwalm wrote:
>>> Hi,
>>>
>>> On 11.08.23 19:02, Tomi Valkeinen wrote:
>>>> On 11/08/2023 19:23, Péter Ujfalusi wrote:
>>>>>
>>>>>
>>>>> On 04/08/2023 13:44, Tomi Valkeinen wrote:
>>>>>> The driver has a few places where it does:
>>>>>>
>>>>>> if (thing_is_enabled_in_config)
>>>>>> 	update_thing_bit_in_hw()
>>>>>>
>>>>>> This means that if the thing is _not_ enabled, the bit never gets
>>>>>> cleared. This affects the h/vsyncs and continuous DSI clock bits.
>>>>>
>>>>> I guess the idea was to keep the reset value unless it needs to be flipped.
>>>>>
>>>>>>
>>>>>> Fix the driver to always update the bit.
>>>>>>
>>>>>> Fixes: ff1ca6397b1d ("drm/bridge: Add tc358768 driver")
>>>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/bridge/tc358768.c | 13 +++++++------
>>>>>>     1 file changed, 7 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
>>>>>> index bc97a837955b..b668f77673c3 100644
>>>>>> --- a/drivers/gpu/drm/bridge/tc358768.c
>>>>>> +++ b/drivers/gpu/drm/bridge/tc358768.c
>>>>>> @@ -794,8 +794,8 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>>>>>>     		val |= BIT(i + 1);
>>>>>>     	tc358768_write(priv, TC358768_HSTXVREGEN, val);
>>>>>>     
>>>>>> -	if (!(mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS))
>>>>>> -		tc358768_write(priv, TC358768_TXOPTIONCNTRL, 0x1);
>>>>>> +	tc358768_write(priv, TC358768_TXOPTIONCNTRL,
>>>>>> +		       (mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) ? 0 : BIT(0));
>>>>>>     
>>>>>>     	/* TXTAGOCNT[26:16] RXTASURECNT[10:0] */
>>>>>>     	val = tc358768_to_ns((lptxcnt + 1) * dsibclk_nsk * 4);
>>>>>> @@ -861,11 +861,12 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>>>>>>     	tc358768_write(priv, TC358768_DSI_HACT, hact);
>>>>>>     
>>>>>>     	/* VSYNC polarity */
>>>>>> -	if (!(mode->flags & DRM_MODE_FLAG_NVSYNC))
>>>>>> -		tc358768_update_bits(priv, TC358768_CONFCTL, BIT(5), BIT(5));
>>>>>> +	tc358768_update_bits(priv, TC358768_CONFCTL, BIT(5),
>>>>>> +			     (mode->flags & DRM_MODE_FLAG_PVSYNC) ? BIT(5) : 0);
>>>>>
>>>>> Was this the reverse before and should be:
>>>>> (mode->flags & DRM_MODE_FLAG_PVSYNC) ? 0 : BIT(5)
>>>>
>>>> Bit 5 is 1 for active high vsync polarity. The test was previously
>>>> !nvsync, i.e. the same as pvsync.
>>>
>>> this statement doesn't seem to be true, since this change causes a
>>> regression on the Asus TF700T. Apparently, !nvsync is true and pvsync is
>>> false in the present case.
>>
>> panasonic_vvx10f004b00_mode in panel_simple.c doesn't seem to have mode
>> flags set. I would say that means the panel doesn't care about the sync
>> polarities (which obviously is not the case), but maybe there's an
>> assumption that if sync polarities are not set, the default is...
>> positive? But I can't find any mention about this.
>>
>> Does it work for you if you set the polarities in
>> panasonic_vvx10f004b00_mode?
> 
> The panel seems to work with either negative or positive H-/Vsync in
> conjunction with the attached patch from Thierry. Currently, the display
> controller is unconditionally programmed for positive H-/Vsync though.
> What should be done in this case?
> 
> BTW, the vendor kernel configures the display controller as well as the
> bridge for negative H-/Vsync.

Also regarding the tegra driver, with a quick look it indeed looks like 
it is configuring hardcoded sync polarities, which is not good. Maybe 
it's time to apply the patch from Thierry, dated 2015 =).

  Tomi
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
index bc97a837955b..b668f77673c3 100644
--- a/drivers/gpu/drm/bridge/tc358768.c
+++ b/drivers/gpu/drm/bridge/tc358768.c
@@ -794,8 +794,8 @@  static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
 		val |= BIT(i + 1);
 	tc358768_write(priv, TC358768_HSTXVREGEN, val);
 
-	if (!(mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS))
-		tc358768_write(priv, TC358768_TXOPTIONCNTRL, 0x1);
+	tc358768_write(priv, TC358768_TXOPTIONCNTRL,
+		       (mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) ? 0 : BIT(0));
 
 	/* TXTAGOCNT[26:16] RXTASURECNT[10:0] */
 	val = tc358768_to_ns((lptxcnt + 1) * dsibclk_nsk * 4);
@@ -861,11 +861,12 @@  static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
 	tc358768_write(priv, TC358768_DSI_HACT, hact);
 
 	/* VSYNC polarity */
-	if (!(mode->flags & DRM_MODE_FLAG_NVSYNC))
-		tc358768_update_bits(priv, TC358768_CONFCTL, BIT(5), BIT(5));
+	tc358768_update_bits(priv, TC358768_CONFCTL, BIT(5),
+			     (mode->flags & DRM_MODE_FLAG_PVSYNC) ? BIT(5) : 0);
+
 	/* HSYNC polarity */
-	if (mode->flags & DRM_MODE_FLAG_PHSYNC)
-		tc358768_update_bits(priv, TC358768_PP_MISC, BIT(0), BIT(0));
+	tc358768_update_bits(priv, TC358768_PP_MISC, BIT(0),
+			     (mode->flags & DRM_MODE_FLAG_PHSYNC) ? BIT(0) : 0);
 
 	/* Start DSI Tx */
 	tc358768_write(priv, TC358768_DSI_START, 0x1);