[PATCH 00/11] drm/bridge: tc358768: Fixes and timings improvements

[PATCH 00/11] drm/bridge: tc358768: Fixes and timings improvements

[Tomi Valkeinen]

This series contains various fixes and cleanups for TC358768. The target
of this work is to get TC358768 working on Toradex's AM62 based board,
which has the following display pipeline:

AM62 DPI -> TC358768 -> LT8912B -> HDMI connector

The two main things the series does:
- Improves DSI HSW, HFP and VSDly calculations
- Adds DRM_BRIDGE_ATTACH_NO_CONNECTOR support

 Tomi

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
Tomi Valkeinen (11):
      drm/bridge: tc358768: Fix use of uninitialized variable
      drm/bridge: tc358768: Fix bit updates
      drm/bridge: tc358768: Cleanup PLL calculations
      drm/bridge: tc358768: Use struct videomode
      drm/bridge: tc358768: Print logical values, not raw register values
      drm/bridge: tc358768: Use dev for dbg prints, not priv->dev
      drm/bridge: tc358768: Rename dsibclk to hsbyteclk
      drm/bridge: tc358768: Clean up clock period code
      drm/bridge: tc358768: Fix tc358768_ns_to_cnt()
      drm/bridge: tc358768: Attempt to fix DSI horizontal timings
      drm/bridge: tc358768: Add DRM_BRIDGE_ATTACH_NO_CONNECTOR support

 drivers/gpu/drm/bridge/tc358768.c | 427 +++++++++++++++++++++++++++-----------
 1 file changed, 309 insertions(+), 118 deletions(-)
---
base-commit: b0e9267d4ccce9be9217337f4bc364ca24cf7f73
change-id: 20230804-tc358768-1b6949ef2e3d

Best regards,
-- 
Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

[PATCH 01/11] drm/bridge: tc358768: Fix use of uninitialized variable

[Tomi Valkeinen]

smatch reports:

drivers/gpu/drm/bridge/tc358768.c:223 tc358768_update_bits() error: uninitialized symbol 'orig'.

Fix this by bailing out from tc358768_update_bits() if the
tc358768_read() produces an error.

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

diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
index 819a4b6ec2a0..bc97a837955b 100644
--- a/drivers/gpu/drm/bridge/tc358768.c
+++ b/drivers/gpu/drm/bridge/tc358768.c
@@ -216,6 +216,10 @@ static void tc358768_update_bits(struct tc358768_priv *priv, u32 reg, u32 mask,
 	u32 tmp, orig;
 
 	tc358768_read(priv, reg, &orig);
+
+	if (priv->error)
+		return;
+
 	tmp = orig & ~mask;
 	tmp |= val & mask;
 	if (tmp != orig)

-- 
2.34.1

Re: [PATCH 01/11] drm/bridge: tc358768: Fix use of uninitialized variable

[Péter Ujfalusi]

On 04/08/2023 13:44, Tomi Valkeinen wrote:
> smatch reports:
> 
> drivers/gpu/drm/bridge/tc358768.c:223 tc358768_update_bits() error: uninitialized symbol 'orig'.
> 
> Fix this by bailing out from tc358768_update_bits() if the
> tc358768_read() produces an error.
> 
> Fixes: ff1ca6397b1d ("drm/bridge: Add tc358768 driver")
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/tc358768.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
> index 819a4b6ec2a0..bc97a837955b 100644
> --- a/drivers/gpu/drm/bridge/tc358768.c
> +++ b/drivers/gpu/drm/bridge/tc358768.c
> @@ -216,6 +216,10 @@ static void tc358768_update_bits(struct tc358768_priv *priv, u32 reg, u32 mask,
>  	u32 tmp, orig;
>  
>  	tc358768_read(priv, reg, &orig);
> +

no need for blank line

> +	if (priv->error)
> +		return;
> +
>  	tmp = orig & ~mask;
>  	tmp |= val & mask;
>  	if (tmp != orig)
> 

-- 
Péter

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

[Tomi Valkeinen]

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(-)

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

-- 
2.34.1

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

[Péter Ujfalusi]

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

-- 
Péter

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

[Tomi Valkeinen]

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

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

[Maxim Schwalm]

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

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

[Tomi Valkeinen]

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

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

[Maxim Schwalm]

[-- Attachment #1: Type: text/plain, Size: 3373 bytes --]

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

[-- Attachment #2: 0001-drm-tegra-rgb-Parameterize-V-and-H-sync-polarities.patch --]
[-- Type: text/x-patch, Size: 1921 bytes --]

From efc4f9108b3f44d3e00146bdb97c906f15e2a068 Mon Sep 17 00:00:00 2001
From: Thierry Reding <treding@nvidia.com>
Date: Mon, 3 Aug 2015 13:34:36 +0200
Subject: [PATCH] drm/tegra: rgb: Parameterize V- and H-sync polarities

The polarities of the V- and H-sync signals are encoded as flags in the
display mode, so use the existing information to setup the signals for
the RGB interface.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/tegra/rgb.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c
index 79566c9ea8ff..cce422c2d6a6 100644
--- a/drivers/gpu/drm/tegra/rgb.c
+++ b/drivers/gpu/drm/tegra/rgb.c
@@ -99,6 +99,7 @@ static void tegra_rgb_encoder_disable(struct drm_encoder *encoder)
 
 static void tegra_rgb_encoder_enable(struct drm_encoder *encoder)
 {
+	struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode;
 	struct tegra_output *output = encoder_to_output(encoder);
 	struct tegra_rgb *rgb = to_rgb(output);
 	u32 value;
@@ -108,10 +109,21 @@ static void tegra_rgb_encoder_enable(struct drm_encoder *encoder)
 	value = DE_SELECT_ACTIVE | DE_CONTROL_NORMAL;
 	tegra_dc_writel(rgb->dc, value, DC_DISP_DATA_ENABLE_OPTIONS);
 
-	/* XXX: parameterize? */
+	/* configure H- and V-sync signal polarities */
 	value = tegra_dc_readl(rgb->dc, DC_COM_PIN_OUTPUT_POLARITY(1));
-	value &= ~LVS_OUTPUT_POLARITY_LOW;
-	value &= ~LHS_OUTPUT_POLARITY_LOW;
+
+	if (mode->flags & DRM_MODE_FLAG_PHSYNC)
+		value &= ~LHS_OUTPUT_POLARITY_LOW;
+
+	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
+		value |= LHS_OUTPUT_POLARITY_LOW;
+
+	if (mode->flags & DRM_MODE_FLAG_PVSYNC)
+		value &= ~LVS_OUTPUT_POLARITY_LOW;
+
+	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
+		value |= LVS_OUTPUT_POLARITY_LOW;
+
 	tegra_dc_writel(rgb->dc, value, DC_COM_PIN_OUTPUT_POLARITY(1));
 
 	/* XXX: parameterize? */
-- 
2.39.2

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

[Tomi Valkeinen]

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

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

[Tomi Valkeinen]

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

[PATCH 03/11] drm/bridge: tc358768: Cleanup PLL calculations

[Tomi Valkeinen]

As is quite common, some of TC358768's PLL register fields are to be
programmed with (value - 1). Specifically, the FBD and PRD, multiplier
and divider, are such fields.

However, what the driver currently does is that it considers that the
formula used for PLL rate calculation is:

RefClk * [(FBD + 1)/ (PRD + 1)] * [1 / (2^FRS)]

where FBD and PRD are values directly from the registers, while a more
sensible way to look at it is:

RefClk * FBD / PRD * (1 / (2^FRS))

and when the FBD and PRD values are written to the registers, they will
be subtracted by one.

Change the driver accordingly, as it simplifies the PLL code.

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

diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
index b668f77673c3..d5831a1236e9 100644
--- a/drivers/gpu/drm/bridge/tc358768.c
+++ b/drivers/gpu/drm/bridge/tc358768.c
@@ -316,7 +316,7 @@ static int tc358768_calc_pll(struct tc358768_priv *priv,
 
 	target_pll = tc358768_pclk_to_pll(priv, mode->clock * 1000);
 
-	/* pll_clk = RefClk * [(FBD + 1)/ (PRD + 1)] * [1 / (2^FRS)] */
+	/* pll_clk = RefClk * FBD / PRD * (1 / (2^FRS)) */
 
 	for (i = 0; i < ARRAY_SIZE(frs_limits); i++)
 		if (target_pll >= frs_limits[i])
@@ -336,19 +336,19 @@ static int tc358768_calc_pll(struct tc358768_priv *priv,
 	best_prd = 0;
 	best_fbd = 0;
 
-	for (prd = 0; prd < 16; ++prd) {
-		u32 divisor = (prd + 1) * (1 << frs);
+	for (prd = 1; prd <= 16; ++prd) {
+		u32 divisor = prd * (1 << frs);
 		u32 fbd;
 
-		for (fbd = 0; fbd < 512; ++fbd) {
+		for (fbd = 1; fbd <= 512; ++fbd) {
 			u32 pll, diff, pll_in;
 
-			pll = (u32)div_u64((u64)refclk * (fbd + 1), divisor);
+			pll = (u32)div_u64((u64)refclk * fbd, divisor);
 
 			if (pll >= max_pll || pll < min_pll)
 				continue;
 
-			pll_in = (u32)div_u64((u64)refclk, prd + 1);
+			pll_in = (u32)div_u64((u64)refclk, prd);
 			if (pll_in < 4000000)
 				continue;
 
@@ -611,7 +611,7 @@ static int tc358768_setup_pll(struct tc358768_priv *priv,
 		mode->clock * 1000);
 
 	/* PRD[15:12] FBD[8:0] */
-	tc358768_write(priv, TC358768_PLLCTL0, (prd << 12) | fbd);
+	tc358768_write(priv, TC358768_PLLCTL0, ((prd - 1) << 12) | (fbd - 1));
 
 	/* FRS[11:10] LBWS[9:8] CKEN[4] RESETB[1] EN[0] */
 	tc358768_write(priv, TC358768_PLLCTL1,

-- 
2.34.1

Re: [PATCH 03/11] drm/bridge: tc358768: Cleanup PLL calculations

[Péter Ujfalusi]

On 04/08/2023 13:44, Tomi Valkeinen wrote:
> As is quite common, some of TC358768's PLL register fields are to be
> programmed with (value - 1). Specifically, the FBD and PRD, multiplier
> and divider, are such fields.
> 
> However, what the driver currently does is that it considers that the
> formula used for PLL rate calculation is:
> 
> RefClk * [(FBD + 1)/ (PRD + 1)] * [1 / (2^FRS)]
> 
> where FBD and PRD are values directly from the registers, while a more
> sensible way to look at it is:
> 
> RefClk * FBD / PRD * (1 / (2^FRS))
> 
> and when the FBD and PRD values are written to the registers, they will
> be subtracted by one.
> 
> Change the driver accordingly, as it simplifies the PLL code.

Reviewed-by: Peter Ujfalusi <peter.ujfalusi@gmail.com>

> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/tc358768.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
> index b668f77673c3..d5831a1236e9 100644
> --- a/drivers/gpu/drm/bridge/tc358768.c
> +++ b/drivers/gpu/drm/bridge/tc358768.c
> @@ -316,7 +316,7 @@ static int tc358768_calc_pll(struct tc358768_priv *priv,
>  
>  	target_pll = tc358768_pclk_to_pll(priv, mode->clock * 1000);
>  
> -	/* pll_clk = RefClk * [(FBD + 1)/ (PRD + 1)] * [1 / (2^FRS)] */
> +	/* pll_clk = RefClk * FBD / PRD * (1 / (2^FRS)) */
>  
>  	for (i = 0; i < ARRAY_SIZE(frs_limits); i++)
>  		if (target_pll >= frs_limits[i])
> @@ -336,19 +336,19 @@ static int tc358768_calc_pll(struct tc358768_priv *priv,
>  	best_prd = 0;
>  	best_fbd = 0;
>  
> -	for (prd = 0; prd < 16; ++prd) {
> -		u32 divisor = (prd + 1) * (1 << frs);
> +	for (prd = 1; prd <= 16; ++prd) {
> +		u32 divisor = prd * (1 << frs);
>  		u32 fbd;
>  
> -		for (fbd = 0; fbd < 512; ++fbd) {
> +		for (fbd = 1; fbd <= 512; ++fbd) {
>  			u32 pll, diff, pll_in;
>  
> -			pll = (u32)div_u64((u64)refclk * (fbd + 1), divisor);
> +			pll = (u32)div_u64((u64)refclk * fbd, divisor);
>  
>  			if (pll >= max_pll || pll < min_pll)
>  				continue;
>  
> -			pll_in = (u32)div_u64((u64)refclk, prd + 1);
> +			pll_in = (u32)div_u64((u64)refclk, prd);
>  			if (pll_in < 4000000)
>  				continue;
>  
> @@ -611,7 +611,7 @@ static int tc358768_setup_pll(struct tc358768_priv *priv,
>  		mode->clock * 1000);
>  
>  	/* PRD[15:12] FBD[8:0] */
> -	tc358768_write(priv, TC358768_PLLCTL0, (prd << 12) | fbd);
> +	tc358768_write(priv, TC358768_PLLCTL0, ((prd - 1) << 12) | (fbd - 1));
>  
>  	/* FRS[11:10] LBWS[9:8] CKEN[4] RESETB[1] EN[0] */
>  	tc358768_write(priv, TC358768_PLLCTL1,
> 

-- 
Péter

[PATCH 04/11] drm/bridge: tc358768: Use struct videomode

[Tomi Valkeinen]

The TC358768 documentation uses HFP, HBP, etc. values to deal with the
video mode, while the driver currently uses the DRM display mode
(htotal, hsync_start, etc).

Change the driver to convert the DRM display mode to struct videomode,
which then allows us to use the same units the documentation uses. This
makes it much easier to work on the code when using the TC358768
documentation as a reference.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/tc358768.c | 45 +++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
index d5831a1236e9..9b633038af33 100644
--- a/drivers/gpu/drm/bridge/tc358768.c
+++ b/drivers/gpu/drm/bridge/tc358768.c
@@ -650,6 +650,7 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
 	u32 dsiclk, dsibclk, video_start;
 	const u32 internal_delay = 40;
 	int ret, i;
+	struct videomode vm;
 
 	if (mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) {
 		dev_warn_once(priv->dev, "Non-continuous mode unimplemented, falling back to continuous\n");
@@ -673,6 +674,8 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
 		return;
 	}
 
+	drm_display_mode_to_videomode(mode, &vm);
+
 	dsiclk = priv->dsiclk;
 	dsibclk = dsiclk / 4;
 
@@ -681,28 +684,28 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
 	switch (dsi_dev->format) {
 	case MIPI_DSI_FMT_RGB888:
 		val |= (0x3 << 4);
-		hact = mode->hdisplay * 3;
-		video_start = (mode->htotal - mode->hsync_start) * 3;
+		hact = vm.hactive * 3;
+		video_start = (vm.hsync_len + vm.hback_porch) * 3;
 		data_type = MIPI_DSI_PACKED_PIXEL_STREAM_24;
 		break;
 	case MIPI_DSI_FMT_RGB666:
 		val |= (0x4 << 4);
-		hact = mode->hdisplay * 3;
-		video_start = (mode->htotal - mode->hsync_start) * 3;
+		hact = vm.hactive * 3;
+		video_start = (vm.hsync_len + vm.hback_porch) * 3;
 		data_type = MIPI_DSI_PACKED_PIXEL_STREAM_18;
 		break;
 
 	case MIPI_DSI_FMT_RGB666_PACKED:
 		val |= (0x4 << 4) | BIT(3);
-		hact = mode->hdisplay * 18 / 8;
-		video_start = (mode->htotal - mode->hsync_start) * 18 / 8;
+		hact = vm.hactive * 18 / 8;
+		video_start = (vm.hsync_len + vm.hback_porch) * 18 / 8;
 		data_type = MIPI_DSI_PIXEL_STREAM_3BYTE_18;
 		break;
 
 	case MIPI_DSI_FMT_RGB565:
 		val |= (0x5 << 4);
-		hact = mode->hdisplay * 2;
-		video_start = (mode->htotal - mode->hsync_start) * 2;
+		hact = vm.hactive * 2;
+		video_start = (vm.hsync_len + vm.hback_porch) * 2;
 		data_type = MIPI_DSI_PACKED_PIXEL_STREAM_16;
 		break;
 	default:
@@ -814,43 +817,43 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
 		tc358768_write(priv, TC358768_DSI_EVENT, 0);
 
 		/* vact */
-		tc358768_write(priv, TC358768_DSI_VACT, mode->vdisplay);
+		tc358768_write(priv, TC358768_DSI_VACT, vm.vactive);
 
 		/* vsw */
-		tc358768_write(priv, TC358768_DSI_VSW,
-			       mode->vsync_end - mode->vsync_start);
+		tc358768_write(priv, TC358768_DSI_VSW, vm.vsync_len);
+
 		/* vbp */
-		tc358768_write(priv, TC358768_DSI_VBPR,
-			       mode->vtotal - mode->vsync_end);
+		tc358768_write(priv, TC358768_DSI_VBPR, vm.vback_porch);
 
 		/* hsw * byteclk * ndl / pclk */
-		val = (u32)div_u64((mode->hsync_end - mode->hsync_start) *
+		val = (u32)div_u64(vm.hsync_len *
 				   ((u64)priv->dsiclk / 4) * priv->dsi_lanes,
-				   mode->clock * 1000);
+				   vm.pixelclock);
 		tc358768_write(priv, TC358768_DSI_HSW, val);
 
 		/* hbp * byteclk * ndl / pclk */
-		val = (u32)div_u64((mode->htotal - mode->hsync_end) *
+		val = (u32)div_u64(vm.hback_porch *
 				   ((u64)priv->dsiclk / 4) * priv->dsi_lanes,
-				   mode->clock * 1000);
+				   vm.pixelclock);
 		tc358768_write(priv, TC358768_DSI_HBPR, val);
 	} else {
 		/* Set event mode */
 		tc358768_write(priv, TC358768_DSI_EVENT, 1);
 
 		/* vact */
-		tc358768_write(priv, TC358768_DSI_VACT, mode->vdisplay);
+		tc358768_write(priv, TC358768_DSI_VACT, vm.vactive);
 
 		/* vsw (+ vbp) */
 		tc358768_write(priv, TC358768_DSI_VSW,
-			       mode->vtotal - mode->vsync_start);
+			       vm.vsync_len + vm.vback_porch);
+
 		/* vbp (not used in event mode) */
 		tc358768_write(priv, TC358768_DSI_VBPR, 0);
 
 		/* (hsw + hbp) * byteclk * ndl / pclk */
-		val = (u32)div_u64((mode->htotal - mode->hsync_start) *
+		val = (u32)div_u64((vm.hsync_len + vm.hback_porch) *
 				   ((u64)priv->dsiclk / 4) * priv->dsi_lanes,
-				   mode->clock * 1000);
+				   vm.pixelclock);
 		tc358768_write(priv, TC358768_DSI_HSW, val);
 
 		/* hbp (not used in event mode) */

-- 
2.34.1

Re: [PATCH 04/11] drm/bridge: tc358768: Use struct videomode

[Péter Ujfalusi]

On 04/08/2023 13:44, Tomi Valkeinen wrote:
> The TC358768 documentation uses HFP, HBP, etc. values to deal with the
> video mode, while the driver currently uses the DRM display mode
> (htotal, hsync_start, etc).
> 
> Change the driver to convert the DRM display mode to struct videomode,
> which then allows us to use the same units the documentation uses. This
> makes it much easier to work on the code when using the TC358768
> documentation as a reference.

Reviewed-by: Peter Ujfalusi <peter.ujfalusi@gmail.com>

> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/tc358768.c | 45 +++++++++++++++++++++------------------
>  1 file changed, 24 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
> index d5831a1236e9..9b633038af33 100644
> --- a/drivers/gpu/drm/bridge/tc358768.c
> +++ b/drivers/gpu/drm/bridge/tc358768.c
> @@ -650,6 +650,7 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>  	u32 dsiclk, dsibclk, video_start;
>  	const u32 internal_delay = 40;
>  	int ret, i;
> +	struct videomode vm;
>  
>  	if (mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) {
>  		dev_warn_once(priv->dev, "Non-continuous mode unimplemented, falling back to continuous\n");
> @@ -673,6 +674,8 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>  		return;
>  	}
>  
> +	drm_display_mode_to_videomode(mode, &vm);
> +
>  	dsiclk = priv->dsiclk;
>  	dsibclk = dsiclk / 4;
>  
> @@ -681,28 +684,28 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>  	switch (dsi_dev->format) {
>  	case MIPI_DSI_FMT_RGB888:
>  		val |= (0x3 << 4);
> -		hact = mode->hdisplay * 3;
> -		video_start = (mode->htotal - mode->hsync_start) * 3;
> +		hact = vm.hactive * 3;
> +		video_start = (vm.hsync_len + vm.hback_porch) * 3;
>  		data_type = MIPI_DSI_PACKED_PIXEL_STREAM_24;
>  		break;
>  	case MIPI_DSI_FMT_RGB666:
>  		val |= (0x4 << 4);
> -		hact = mode->hdisplay * 3;
> -		video_start = (mode->htotal - mode->hsync_start) * 3;
> +		hact = vm.hactive * 3;
> +		video_start = (vm.hsync_len + vm.hback_porch) * 3;
>  		data_type = MIPI_DSI_PACKED_PIXEL_STREAM_18;
>  		break;
>  
>  	case MIPI_DSI_FMT_RGB666_PACKED:
>  		val |= (0x4 << 4) | BIT(3);
> -		hact = mode->hdisplay * 18 / 8;
> -		video_start = (mode->htotal - mode->hsync_start) * 18 / 8;
> +		hact = vm.hactive * 18 / 8;
> +		video_start = (vm.hsync_len + vm.hback_porch) * 18 / 8;
>  		data_type = MIPI_DSI_PIXEL_STREAM_3BYTE_18;
>  		break;
>  
>  	case MIPI_DSI_FMT_RGB565:
>  		val |= (0x5 << 4);
> -		hact = mode->hdisplay * 2;
> -		video_start = (mode->htotal - mode->hsync_start) * 2;
> +		hact = vm.hactive * 2;
> +		video_start = (vm.hsync_len + vm.hback_porch) * 2;
>  		data_type = MIPI_DSI_PACKED_PIXEL_STREAM_16;
>  		break;
>  	default:
> @@ -814,43 +817,43 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>  		tc358768_write(priv, TC358768_DSI_EVENT, 0);
>  
>  		/* vact */
> -		tc358768_write(priv, TC358768_DSI_VACT, mode->vdisplay);
> +		tc358768_write(priv, TC358768_DSI_VACT, vm.vactive);
>  
>  		/* vsw */
> -		tc358768_write(priv, TC358768_DSI_VSW,
> -			       mode->vsync_end - mode->vsync_start);
> +		tc358768_write(priv, TC358768_DSI_VSW, vm.vsync_len);
> +
>  		/* vbp */
> -		tc358768_write(priv, TC358768_DSI_VBPR,
> -			       mode->vtotal - mode->vsync_end);
> +		tc358768_write(priv, TC358768_DSI_VBPR, vm.vback_porch);
>  
>  		/* hsw * byteclk * ndl / pclk */
> -		val = (u32)div_u64((mode->hsync_end - mode->hsync_start) *
> +		val = (u32)div_u64(vm.hsync_len *
>  				   ((u64)priv->dsiclk / 4) * priv->dsi_lanes,
> -				   mode->clock * 1000);
> +				   vm.pixelclock);
>  		tc358768_write(priv, TC358768_DSI_HSW, val);
>  
>  		/* hbp * byteclk * ndl / pclk */
> -		val = (u32)div_u64((mode->htotal - mode->hsync_end) *
> +		val = (u32)div_u64(vm.hback_porch *
>  				   ((u64)priv->dsiclk / 4) * priv->dsi_lanes,
> -				   mode->clock * 1000);
> +				   vm.pixelclock);
>  		tc358768_write(priv, TC358768_DSI_HBPR, val);
>  	} else {
>  		/* Set event mode */
>  		tc358768_write(priv, TC358768_DSI_EVENT, 1);
>  
>  		/* vact */
> -		tc358768_write(priv, TC358768_DSI_VACT, mode->vdisplay);
> +		tc358768_write(priv, TC358768_DSI_VACT, vm.vactive);
>  
>  		/* vsw (+ vbp) */
>  		tc358768_write(priv, TC358768_DSI_VSW,
> -			       mode->vtotal - mode->vsync_start);
> +			       vm.vsync_len + vm.vback_porch);
> +
>  		/* vbp (not used in event mode) */
>  		tc358768_write(priv, TC358768_DSI_VBPR, 0);
>  
>  		/* (hsw + hbp) * byteclk * ndl / pclk */
> -		val = (u32)div_u64((mode->htotal - mode->hsync_start) *
> +		val = (u32)div_u64((vm.hsync_len + vm.hback_porch) *
>  				   ((u64)priv->dsiclk / 4) * priv->dsi_lanes,
> -				   mode->clock * 1000);
> +				   vm.pixelclock);
>  		tc358768_write(priv, TC358768_DSI_HSW, val);
>  
>  		/* hbp (not used in event mode) */
> 

-- 
Péter

[PATCH 05/11] drm/bridge: tc358768: Print logical values, not raw register values

[Tomi Valkeinen]

The driver debug prints DSI related timings as raw register values in
hex. It is much more useful to see the "logical" value of the timing,
not the register value.

Change the prints to print the values separately, in case a single
register contains multiple values, and use %u to have it in a more human
consumable form.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/tc358768.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
index 9b633038af33..0ef51d04bb21 100644
--- a/drivers/gpu/drm/bridge/tc358768.c
+++ b/drivers/gpu/drm/bridge/tc358768.c
@@ -739,57 +739,59 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
 
 	/* LP11 > 100us for D-PHY Rx Init */
 	val = tc358768_ns_to_cnt(100 * 1000, dsibclk_nsk) - 1;
-	dev_dbg(priv->dev, "LINEINITCNT: 0x%x\n", val);
+	dev_dbg(priv->dev, "LINEINITCNT: %u\n", val);
 	tc358768_write(priv, TC358768_LINEINITCNT, val);
 
 	/* LPTimeCnt > 50ns */
 	val = tc358768_ns_to_cnt(50, dsibclk_nsk) - 1;
 	lptxcnt = val;
-	dev_dbg(priv->dev, "LPTXTIMECNT: 0x%x\n", val);
+	dev_dbg(priv->dev, "LPTXTIMECNT: %u\n", val);
 	tc358768_write(priv, TC358768_LPTXTIMECNT, val);
 
 	/* 38ns < TCLK_PREPARE < 95ns */
 	val = tc358768_ns_to_cnt(65, dsibclk_nsk) - 1;
+	dev_dbg(priv->dev, "TCLK_PREPARECNT %u\n", val);
 	/* TCLK_PREPARE + TCLK_ZERO > 300ns */
 	val2 = tc358768_ns_to_cnt(300 - tc358768_to_ns(2 * ui_nsk),
 				  dsibclk_nsk) - 2;
+	dev_dbg(priv->dev, "TCLK_ZEROCNT %u\n", val2);
 	val |= val2 << 8;
-	dev_dbg(priv->dev, "TCLK_HEADERCNT: 0x%x\n", val);
 	tc358768_write(priv, TC358768_TCLK_HEADERCNT, val);
 
 	/* TCLK_TRAIL > 60ns AND TEOT <= 105 ns + 12*UI */
 	raw_val = tc358768_ns_to_cnt(60 + tc358768_to_ns(2 * ui_nsk), dsibclk_nsk) - 5;
 	val = clamp(raw_val, 0, 127);
-	dev_dbg(priv->dev, "TCLK_TRAILCNT: 0x%x\n", val);
+	dev_dbg(priv->dev, "TCLK_TRAILCNT: %u\n", val);
 	tc358768_write(priv, TC358768_TCLK_TRAILCNT, val);
 
 	/* 40ns + 4*UI < THS_PREPARE < 85ns + 6*UI */
 	val = 50 + tc358768_to_ns(4 * ui_nsk);
 	val = tc358768_ns_to_cnt(val, dsibclk_nsk) - 1;
+	dev_dbg(priv->dev, "THS_PREPARECNT %u\n", val);
 	/* THS_PREPARE + THS_ZERO > 145ns + 10*UI */
 	raw_val = tc358768_ns_to_cnt(145 - tc358768_to_ns(3 * ui_nsk), dsibclk_nsk) - 10;
 	val2 = clamp(raw_val, 0, 127);
+	dev_dbg(priv->dev, "THS_ZEROCNT %u\n", val2);
 	val |= val2 << 8;
-	dev_dbg(priv->dev, "THS_HEADERCNT: 0x%x\n", val);
 	tc358768_write(priv, TC358768_THS_HEADERCNT, val);
 
 	/* TWAKEUP > 1ms in lptxcnt steps */
 	val = tc358768_ns_to_cnt(1020000, dsibclk_nsk);
 	val = val / (lptxcnt + 1) - 1;
-	dev_dbg(priv->dev, "TWAKEUP: 0x%x\n", val);
+	dev_dbg(priv->dev, "TWAKEUP: %u\n", val);
 	tc358768_write(priv, TC358768_TWAKEUP, val);
 
 	/* TCLK_POSTCNT > 60ns + 52*UI */
 	val = tc358768_ns_to_cnt(60 + tc358768_to_ns(52 * ui_nsk),
 				 dsibclk_nsk) - 3;
-	dev_dbg(priv->dev, "TCLK_POSTCNT: 0x%x\n", val);
+	dev_dbg(priv->dev, "TCLK_POSTCNT: %u\n", val);
 	tc358768_write(priv, TC358768_TCLK_POSTCNT, val);
 
 	/* max(60ns + 4*UI, 8*UI) < THS_TRAILCNT < 105ns + 12*UI */
 	raw_val = tc358768_ns_to_cnt(60 + tc358768_to_ns(18 * ui_nsk),
 				     dsibclk_nsk) - 4;
 	val = clamp(raw_val, 0, 15);
-	dev_dbg(priv->dev, "THS_TRAILCNT: 0x%x\n", val);
+	dev_dbg(priv->dev, "THS_TRAILCNT: %u\n", val);
 	tc358768_write(priv, TC358768_THS_TRAILCNT, val);
 
 	val = BIT(0);
@@ -803,10 +805,11 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
 	/* TXTAGOCNT[26:16] RXTASURECNT[10:0] */
 	val = tc358768_to_ns((lptxcnt + 1) * dsibclk_nsk * 4);
 	val = tc358768_ns_to_cnt(val, dsibclk_nsk) / 4 - 1;
+	dev_dbg(priv->dev, "TXTAGOCNT: %u\n", val);
 	val2 = tc358768_ns_to_cnt(tc358768_to_ns((lptxcnt + 1) * dsibclk_nsk),
 				  dsibclk_nsk) - 2;
+	dev_dbg(priv->dev, "RXTASURECNT: %u\n", val2);
 	val = val << 16 | val2;
-	dev_dbg(priv->dev, "BTACNTRL1: 0x%x\n", val);
 	tc358768_write(priv, TC358768_BTACNTRL1, val);
 
 	/* START[0] */

-- 
2.34.1

Re: [PATCH 05/11] drm/bridge: tc358768: Print logical values, not raw register values

[Péter Ujfalusi]

On 04/08/2023 13:44, Tomi Valkeinen wrote:
> The driver debug prints DSI related timings as raw register values in
> hex. It is much more useful to see the "logical" value of the timing,
> not the register value.

I'm a bit confused by the term 'logical' value, I think you meant
decimal, easier to read by humans numbers.

> Change the prints to print the values separately, in case a single
> register contains multiple values, and use %u to have it in a more human
> consumable form.

But, yes, decimal is better for the dmesg, as I recall I had a tool
which was using hex numbers so it was better to have the prints also in hex.

Reviewed-by: Peter Ujfalusi <peter.ujfalusi@gmail.com>

> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/tc358768.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
> index 9b633038af33..0ef51d04bb21 100644
> --- a/drivers/gpu/drm/bridge/tc358768.c
> +++ b/drivers/gpu/drm/bridge/tc358768.c
> @@ -739,57 +739,59 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>  
>  	/* LP11 > 100us for D-PHY Rx Init */
>  	val = tc358768_ns_to_cnt(100 * 1000, dsibclk_nsk) - 1;
> -	dev_dbg(priv->dev, "LINEINITCNT: 0x%x\n", val);
> +	dev_dbg(priv->dev, "LINEINITCNT: %u\n", val);
>  	tc358768_write(priv, TC358768_LINEINITCNT, val);
>  
>  	/* LPTimeCnt > 50ns */
>  	val = tc358768_ns_to_cnt(50, dsibclk_nsk) - 1;
>  	lptxcnt = val;
> -	dev_dbg(priv->dev, "LPTXTIMECNT: 0x%x\n", val);
> +	dev_dbg(priv->dev, "LPTXTIMECNT: %u\n", val);
>  	tc358768_write(priv, TC358768_LPTXTIMECNT, val);
>  
>  	/* 38ns < TCLK_PREPARE < 95ns */
>  	val = tc358768_ns_to_cnt(65, dsibclk_nsk) - 1;
> +	dev_dbg(priv->dev, "TCLK_PREPARECNT %u\n", val);
>  	/* TCLK_PREPARE + TCLK_ZERO > 300ns */
>  	val2 = tc358768_ns_to_cnt(300 - tc358768_to_ns(2 * ui_nsk),
>  				  dsibclk_nsk) - 2;
> +	dev_dbg(priv->dev, "TCLK_ZEROCNT %u\n", val2);
>  	val |= val2 << 8;
> -	dev_dbg(priv->dev, "TCLK_HEADERCNT: 0x%x\n", val);
>  	tc358768_write(priv, TC358768_TCLK_HEADERCNT, val);
>  
>  	/* TCLK_TRAIL > 60ns AND TEOT <= 105 ns + 12*UI */
>  	raw_val = tc358768_ns_to_cnt(60 + tc358768_to_ns(2 * ui_nsk), dsibclk_nsk) - 5;
>  	val = clamp(raw_val, 0, 127);
> -	dev_dbg(priv->dev, "TCLK_TRAILCNT: 0x%x\n", val);
> +	dev_dbg(priv->dev, "TCLK_TRAILCNT: %u\n", val);
>  	tc358768_write(priv, TC358768_TCLK_TRAILCNT, val);
>  
>  	/* 40ns + 4*UI < THS_PREPARE < 85ns + 6*UI */
>  	val = 50 + tc358768_to_ns(4 * ui_nsk);
>  	val = tc358768_ns_to_cnt(val, dsibclk_nsk) - 1;
> +	dev_dbg(priv->dev, "THS_PREPARECNT %u\n", val);
>  	/* THS_PREPARE + THS_ZERO > 145ns + 10*UI */
>  	raw_val = tc358768_ns_to_cnt(145 - tc358768_to_ns(3 * ui_nsk), dsibclk_nsk) - 10;
>  	val2 = clamp(raw_val, 0, 127);
> +	dev_dbg(priv->dev, "THS_ZEROCNT %u\n", val2);
>  	val |= val2 << 8;
> -	dev_dbg(priv->dev, "THS_HEADERCNT: 0x%x\n", val);
>  	tc358768_write(priv, TC358768_THS_HEADERCNT, val);
>  
>  	/* TWAKEUP > 1ms in lptxcnt steps */
>  	val = tc358768_ns_to_cnt(1020000, dsibclk_nsk);
>  	val = val / (lptxcnt + 1) - 1;
> -	dev_dbg(priv->dev, "TWAKEUP: 0x%x\n", val);
> +	dev_dbg(priv->dev, "TWAKEUP: %u\n", val);
>  	tc358768_write(priv, TC358768_TWAKEUP, val);
>  
>  	/* TCLK_POSTCNT > 60ns + 52*UI */
>  	val = tc358768_ns_to_cnt(60 + tc358768_to_ns(52 * ui_nsk),
>  				 dsibclk_nsk) - 3;
> -	dev_dbg(priv->dev, "TCLK_POSTCNT: 0x%x\n", val);
> +	dev_dbg(priv->dev, "TCLK_POSTCNT: %u\n", val);
>  	tc358768_write(priv, TC358768_TCLK_POSTCNT, val);
>  
>  	/* max(60ns + 4*UI, 8*UI) < THS_TRAILCNT < 105ns + 12*UI */
>  	raw_val = tc358768_ns_to_cnt(60 + tc358768_to_ns(18 * ui_nsk),
>  				     dsibclk_nsk) - 4;
>  	val = clamp(raw_val, 0, 15);
> -	dev_dbg(priv->dev, "THS_TRAILCNT: 0x%x\n", val);
> +	dev_dbg(priv->dev, "THS_TRAILCNT: %u\n", val);
>  	tc358768_write(priv, TC358768_THS_TRAILCNT, val);
>  
>  	val = BIT(0);
> @@ -803,10 +805,11 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>  	/* TXTAGOCNT[26:16] RXTASURECNT[10:0] */
>  	val = tc358768_to_ns((lptxcnt + 1) * dsibclk_nsk * 4);
>  	val = tc358768_ns_to_cnt(val, dsibclk_nsk) / 4 - 1;
> +	dev_dbg(priv->dev, "TXTAGOCNT: %u\n", val);
>  	val2 = tc358768_ns_to_cnt(tc358768_to_ns((lptxcnt + 1) * dsibclk_nsk),
>  				  dsibclk_nsk) - 2;
> +	dev_dbg(priv->dev, "RXTASURECNT: %u\n", val2);
>  	val = val << 16 | val2;
> -	dev_dbg(priv->dev, "BTACNTRL1: 0x%x\n", val);
>  	tc358768_write(priv, TC358768_BTACNTRL1, val);
>  
>  	/* START[0] */
> 

-- 
Péter

Re: [PATCH 05/11] drm/bridge: tc358768: Print logical values, not raw register values

[Tomi Valkeinen]

On 11/08/2023 19:31, Péter Ujfalusi wrote:
> 
> 
> On 04/08/2023 13:44, Tomi Valkeinen wrote:
>> The driver debug prints DSI related timings as raw register values in
>> hex. It is much more useful to see the "logical" value of the timing,
>> not the register value.
> 
> I'm a bit confused by the term 'logical' value, I think you meant
> decimal, easier to read by humans numbers.

Not just decimal. Previously the code printed the register values, which 
e.g. could contain two values ORed together. So, with "logical" I just 
meant the "real" value, instead of "register-encoded".

>> Change the prints to print the values separately, in case a single
>> register contains multiple values, and use %u to have it in a more human
>> consumable form.
> 
> But, yes, decimal is better for the dmesg, as I recall I had a tool
> which was using hex numbers so it was better to have the prints also in hex.
> 
> Reviewed-by: Peter Ujfalusi <peter.ujfalusi@gmail.com>
> 
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/gpu/drm/bridge/tc358768.c | 21 ++++++++++++---------
>>   1 file changed, 12 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
>> index 9b633038af33..0ef51d04bb21 100644
>> --- a/drivers/gpu/drm/bridge/tc358768.c
>> +++ b/drivers/gpu/drm/bridge/tc358768.c
>> @@ -739,57 +739,59 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>>   
>>   	/* LP11 > 100us for D-PHY Rx Init */
>>   	val = tc358768_ns_to_cnt(100 * 1000, dsibclk_nsk) - 1;
>> -	dev_dbg(priv->dev, "LINEINITCNT: 0x%x\n", val);
>> +	dev_dbg(priv->dev, "LINEINITCNT: %u\n", val);
>>   	tc358768_write(priv, TC358768_LINEINITCNT, val);
>>   
>>   	/* LPTimeCnt > 50ns */
>>   	val = tc358768_ns_to_cnt(50, dsibclk_nsk) - 1;
>>   	lptxcnt = val;
>> -	dev_dbg(priv->dev, "LPTXTIMECNT: 0x%x\n", val);
>> +	dev_dbg(priv->dev, "LPTXTIMECNT: %u\n", val);
>>   	tc358768_write(priv, TC358768_LPTXTIMECNT, val);
>>   
>>   	/* 38ns < TCLK_PREPARE < 95ns */
>>   	val = tc358768_ns_to_cnt(65, dsibclk_nsk) - 1;
>> +	dev_dbg(priv->dev, "TCLK_PREPARECNT %u\n", val);
>>   	/* TCLK_PREPARE + TCLK_ZERO > 300ns */
>>   	val2 = tc358768_ns_to_cnt(300 - tc358768_to_ns(2 * ui_nsk),
>>   				  dsibclk_nsk) - 2;
>> +	dev_dbg(priv->dev, "TCLK_ZEROCNT %u\n", val2);
>>   	val |= val2 << 8;
>> -	dev_dbg(priv->dev, "TCLK_HEADERCNT: 0x%x\n", val);
>>   	tc358768_write(priv, TC358768_TCLK_HEADERCNT, val);
>>   
>>   	/* TCLK_TRAIL > 60ns AND TEOT <= 105 ns + 12*UI */
>>   	raw_val = tc358768_ns_to_cnt(60 + tc358768_to_ns(2 * ui_nsk), dsibclk_nsk) - 5;
>>   	val = clamp(raw_val, 0, 127);
>> -	dev_dbg(priv->dev, "TCLK_TRAILCNT: 0x%x\n", val);
>> +	dev_dbg(priv->dev, "TCLK_TRAILCNT: %u\n", val);
>>   	tc358768_write(priv, TC358768_TCLK_TRAILCNT, val);
>>   
>>   	/* 40ns + 4*UI < THS_PREPARE < 85ns + 6*UI */
>>   	val = 50 + tc358768_to_ns(4 * ui_nsk);
>>   	val = tc358768_ns_to_cnt(val, dsibclk_nsk) - 1;
>> +	dev_dbg(priv->dev, "THS_PREPARECNT %u\n", val);
>>   	/* THS_PREPARE + THS_ZERO > 145ns + 10*UI */
>>   	raw_val = tc358768_ns_to_cnt(145 - tc358768_to_ns(3 * ui_nsk), dsibclk_nsk) - 10;
>>   	val2 = clamp(raw_val, 0, 127);
>> +	dev_dbg(priv->dev, "THS_ZEROCNT %u\n", val2);
>>   	val |= val2 << 8;
>> -	dev_dbg(priv->dev, "THS_HEADERCNT: 0x%x\n", val);
>>   	tc358768_write(priv, TC358768_THS_HEADERCNT, val);
>>   
>>   	/* TWAKEUP > 1ms in lptxcnt steps */
>>   	val = tc358768_ns_to_cnt(1020000, dsibclk_nsk);
>>   	val = val / (lptxcnt + 1) - 1;
>> -	dev_dbg(priv->dev, "TWAKEUP: 0x%x\n", val);
>> +	dev_dbg(priv->dev, "TWAKEUP: %u\n", val);
>>   	tc358768_write(priv, TC358768_TWAKEUP, val);
>>   
>>   	/* TCLK_POSTCNT > 60ns + 52*UI */
>>   	val = tc358768_ns_to_cnt(60 + tc358768_to_ns(52 * ui_nsk),
>>   				 dsibclk_nsk) - 3;
>> -	dev_dbg(priv->dev, "TCLK_POSTCNT: 0x%x\n", val);
>> +	dev_dbg(priv->dev, "TCLK_POSTCNT: %u\n", val);
>>   	tc358768_write(priv, TC358768_TCLK_POSTCNT, val);
>>   
>>   	/* max(60ns + 4*UI, 8*UI) < THS_TRAILCNT < 105ns + 12*UI */
>>   	raw_val = tc358768_ns_to_cnt(60 + tc358768_to_ns(18 * ui_nsk),
>>   				     dsibclk_nsk) - 4;
>>   	val = clamp(raw_val, 0, 15);
>> -	dev_dbg(priv->dev, "THS_TRAILCNT: 0x%x\n", val);
>> +	dev_dbg(priv->dev, "THS_TRAILCNT: %u\n", val);
>>   	tc358768_write(priv, TC358768_THS_TRAILCNT, val);
>>   
>>   	val = BIT(0);
>> @@ -803,10 +805,11 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>>   	/* TXTAGOCNT[26:16] RXTASURECNT[10:0] */
>>   	val = tc358768_to_ns((lptxcnt + 1) * dsibclk_nsk * 4);
>>   	val = tc358768_ns_to_cnt(val, dsibclk_nsk) / 4 - 1;
>> +	dev_dbg(priv->dev, "TXTAGOCNT: %u\n", val);
>>   	val2 = tc358768_ns_to_cnt(tc358768_to_ns((lptxcnt + 1) * dsibclk_nsk),
>>   				  dsibclk_nsk) - 2;
>> +	dev_dbg(priv->dev, "RXTASURECNT: %u\n", val2);
>>   	val = val << 16 | val2;
>> -	dev_dbg(priv->dev, "BTACNTRL1: 0x%x\n", val);
>>   	tc358768_write(priv, TC358768_BTACNTRL1, val);
>>   
>>   	/* START[0] */
>>
> 

[PATCH 06/11] drm/bridge: tc358768: Use dev for dbg prints, not priv->dev

[Tomi Valkeinen]

Simplify the code by capturing the priv->dev value to dev variable, and
use it.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/tc358768.c | 41 ++++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
index 0ef51d04bb21..3266c08d9bf1 100644
--- a/drivers/gpu/drm/bridge/tc358768.c
+++ b/drivers/gpu/drm/bridge/tc358768.c
@@ -651,9 +651,10 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
 	const u32 internal_delay = 40;
 	int ret, i;
 	struct videomode vm;
+	struct device *dev = priv->dev;
 
 	if (mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) {
-		dev_warn_once(priv->dev, "Non-continuous mode unimplemented, falling back to continuous\n");
+		dev_warn_once(dev, "Non-continuous mode unimplemented, falling back to continuous\n");
 		mode_flags &= ~MIPI_DSI_CLOCK_NON_CONTINUOUS;
 	}
 
@@ -661,7 +662,7 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
 
 	ret = tc358768_sw_reset(priv);
 	if (ret) {
-		dev_err(priv->dev, "Software reset failed: %d\n", ret);
+		dev_err(dev, "Software reset failed: %d\n", ret);
 		tc358768_hw_disable(priv);
 		return;
 	}
@@ -669,7 +670,7 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
 	mode = &bridge->encoder->crtc->state->adjusted_mode;
 	ret = tc358768_setup_pll(priv, mode);
 	if (ret) {
-		dev_err(priv->dev, "PLL setup failed: %d\n", ret);
+		dev_err(dev, "PLL setup failed: %d\n", ret);
 		tc358768_hw_disable(priv);
 		return;
 	}
@@ -709,7 +710,7 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
 		data_type = MIPI_DSI_PACKED_PIXEL_STREAM_16;
 		break;
 	default:
-		dev_err(priv->dev, "Invalid data format (%u)\n",
+		dev_err(dev, "Invalid data format (%u)\n",
 			dsi_dev->format);
 		tc358768_hw_disable(priv);
 		return;
@@ -733,65 +734,65 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
 				  dsibclk);
 	dsiclk_nsk = (u32)div_u64((u64)1000000000 * TC358768_PRECISION, dsiclk);
 	ui_nsk = dsiclk_nsk / 2;
-	dev_dbg(priv->dev, "dsiclk_nsk: %u\n", dsiclk_nsk);
-	dev_dbg(priv->dev, "ui_nsk: %u\n", ui_nsk);
-	dev_dbg(priv->dev, "dsibclk_nsk: %u\n", dsibclk_nsk);
+	dev_dbg(dev, "dsiclk_nsk: %u\n", dsiclk_nsk);
+	dev_dbg(dev, "ui_nsk: %u\n", ui_nsk);
+	dev_dbg(dev, "dsibclk_nsk: %u\n", dsibclk_nsk);
 
 	/* LP11 > 100us for D-PHY Rx Init */
 	val = tc358768_ns_to_cnt(100 * 1000, dsibclk_nsk) - 1;
-	dev_dbg(priv->dev, "LINEINITCNT: %u\n", val);
+	dev_dbg(dev, "LINEINITCNT: %u\n", val);
 	tc358768_write(priv, TC358768_LINEINITCNT, val);
 
 	/* LPTimeCnt > 50ns */
 	val = tc358768_ns_to_cnt(50, dsibclk_nsk) - 1;
 	lptxcnt = val;
-	dev_dbg(priv->dev, "LPTXTIMECNT: %u\n", val);
+	dev_dbg(dev, "LPTXTIMECNT: %u\n", val);
 	tc358768_write(priv, TC358768_LPTXTIMECNT, val);
 
 	/* 38ns < TCLK_PREPARE < 95ns */
 	val = tc358768_ns_to_cnt(65, dsibclk_nsk) - 1;
-	dev_dbg(priv->dev, "TCLK_PREPARECNT %u\n", val);
+	dev_dbg(dev, "TCLK_PREPARECNT %u\n", val);
 	/* TCLK_PREPARE + TCLK_ZERO > 300ns */
 	val2 = tc358768_ns_to_cnt(300 - tc358768_to_ns(2 * ui_nsk),
 				  dsibclk_nsk) - 2;
-	dev_dbg(priv->dev, "TCLK_ZEROCNT %u\n", val2);
+	dev_dbg(dev, "TCLK_ZEROCNT %u\n", val2);
 	val |= val2 << 8;
 	tc358768_write(priv, TC358768_TCLK_HEADERCNT, val);
 
 	/* TCLK_TRAIL > 60ns AND TEOT <= 105 ns + 12*UI */
 	raw_val = tc358768_ns_to_cnt(60 + tc358768_to_ns(2 * ui_nsk), dsibclk_nsk) - 5;
 	val = clamp(raw_val, 0, 127);
-	dev_dbg(priv->dev, "TCLK_TRAILCNT: %u\n", val);
+	dev_dbg(dev, "TCLK_TRAILCNT: %u\n", val);
 	tc358768_write(priv, TC358768_TCLK_TRAILCNT, val);
 
 	/* 40ns + 4*UI < THS_PREPARE < 85ns + 6*UI */
 	val = 50 + tc358768_to_ns(4 * ui_nsk);
 	val = tc358768_ns_to_cnt(val, dsibclk_nsk) - 1;
-	dev_dbg(priv->dev, "THS_PREPARECNT %u\n", val);
+	dev_dbg(dev, "THS_PREPARECNT %u\n", val);
 	/* THS_PREPARE + THS_ZERO > 145ns + 10*UI */
 	raw_val = tc358768_ns_to_cnt(145 - tc358768_to_ns(3 * ui_nsk), dsibclk_nsk) - 10;
 	val2 = clamp(raw_val, 0, 127);
-	dev_dbg(priv->dev, "THS_ZEROCNT %u\n", val2);
+	dev_dbg(dev, "THS_ZEROCNT %u\n", val2);
 	val |= val2 << 8;
 	tc358768_write(priv, TC358768_THS_HEADERCNT, val);
 
 	/* TWAKEUP > 1ms in lptxcnt steps */
 	val = tc358768_ns_to_cnt(1020000, dsibclk_nsk);
 	val = val / (lptxcnt + 1) - 1;
-	dev_dbg(priv->dev, "TWAKEUP: %u\n", val);
+	dev_dbg(dev, "TWAKEUP: %u\n", val);
 	tc358768_write(priv, TC358768_TWAKEUP, val);
 
 	/* TCLK_POSTCNT > 60ns + 52*UI */
 	val = tc358768_ns_to_cnt(60 + tc358768_to_ns(52 * ui_nsk),
 				 dsibclk_nsk) - 3;
-	dev_dbg(priv->dev, "TCLK_POSTCNT: %u\n", val);
+	dev_dbg(dev, "TCLK_POSTCNT: %u\n", val);
 	tc358768_write(priv, TC358768_TCLK_POSTCNT, val);
 
 	/* max(60ns + 4*UI, 8*UI) < THS_TRAILCNT < 105ns + 12*UI */
 	raw_val = tc358768_ns_to_cnt(60 + tc358768_to_ns(18 * ui_nsk),
 				     dsibclk_nsk) - 4;
 	val = clamp(raw_val, 0, 15);
-	dev_dbg(priv->dev, "THS_TRAILCNT: %u\n", val);
+	dev_dbg(dev, "THS_TRAILCNT: %u\n", val);
 	tc358768_write(priv, TC358768_THS_TRAILCNT, val);
 
 	val = BIT(0);
@@ -805,10 +806,10 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
 	/* TXTAGOCNT[26:16] RXTASURECNT[10:0] */
 	val = tc358768_to_ns((lptxcnt + 1) * dsibclk_nsk * 4);
 	val = tc358768_ns_to_cnt(val, dsibclk_nsk) / 4 - 1;
-	dev_dbg(priv->dev, "TXTAGOCNT: %u\n", val);
+	dev_dbg(dev, "TXTAGOCNT: %u\n", val);
 	val2 = tc358768_ns_to_cnt(tc358768_to_ns((lptxcnt + 1) * dsibclk_nsk),
 				  dsibclk_nsk) - 2;
-	dev_dbg(priv->dev, "RXTASURECNT: %u\n", val2);
+	dev_dbg(dev, "RXTASURECNT: %u\n", val2);
 	val = val << 16 | val2;
 	tc358768_write(priv, TC358768_BTACNTRL1, val);
 
@@ -902,7 +903,7 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
 
 	ret = tc358768_clear_error(priv);
 	if (ret) {
-		dev_err(priv->dev, "Bridge pre_enable failed: %d\n", ret);
+		dev_err(dev, "Bridge pre_enable failed: %d\n", ret);
 		tc358768_bridge_disable(bridge);
 		tc358768_bridge_post_disable(bridge);
 	}

-- 
2.34.1

Re: [PATCH 06/11] drm/bridge: tc358768: Use dev for dbg prints, not priv->dev

[Péter Ujfalusi]

On 04/08/2023 13:44, Tomi Valkeinen wrote:
> Simplify the code by capturing the priv->dev value to dev variable, and
> use it.

Reviewed-by: Peter Ujfalusi <peter.ujfalusi@gmail.com>

> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/tc358768.c | 41 ++++++++++++++++++++-------------------
>  1 file changed, 21 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
> index 0ef51d04bb21..3266c08d9bf1 100644
> --- a/drivers/gpu/drm/bridge/tc358768.c
> +++ b/drivers/gpu/drm/bridge/tc358768.c
> @@ -651,9 +651,10 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>  	const u32 internal_delay = 40;
>  	int ret, i;
>  	struct videomode vm;
> +	struct device *dev = priv->dev;
>  
>  	if (mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) {
> -		dev_warn_once(priv->dev, "Non-continuous mode unimplemented, falling back to continuous\n");
> +		dev_warn_once(dev, "Non-continuous mode unimplemented, falling back to continuous\n");
>  		mode_flags &= ~MIPI_DSI_CLOCK_NON_CONTINUOUS;
>  	}
>  
> @@ -661,7 +662,7 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>  
>  	ret = tc358768_sw_reset(priv);
>  	if (ret) {
> -		dev_err(priv->dev, "Software reset failed: %d\n", ret);
> +		dev_err(dev, "Software reset failed: %d\n", ret);
>  		tc358768_hw_disable(priv);
>  		return;
>  	}
> @@ -669,7 +670,7 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>  	mode = &bridge->encoder->crtc->state->adjusted_mode;
>  	ret = tc358768_setup_pll(priv, mode);
>  	if (ret) {
> -		dev_err(priv->dev, "PLL setup failed: %d\n", ret);
> +		dev_err(dev, "PLL setup failed: %d\n", ret);
>  		tc358768_hw_disable(priv);
>  		return;
>  	}
> @@ -709,7 +710,7 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>  		data_type = MIPI_DSI_PACKED_PIXEL_STREAM_16;
>  		break;
>  	default:
> -		dev_err(priv->dev, "Invalid data format (%u)\n",
> +		dev_err(dev, "Invalid data format (%u)\n",
>  			dsi_dev->format);
>  		tc358768_hw_disable(priv);
>  		return;
> @@ -733,65 +734,65 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>  				  dsibclk);
>  	dsiclk_nsk = (u32)div_u64((u64)1000000000 * TC358768_PRECISION, dsiclk);
>  	ui_nsk = dsiclk_nsk / 2;
> -	dev_dbg(priv->dev, "dsiclk_nsk: %u\n", dsiclk_nsk);
> -	dev_dbg(priv->dev, "ui_nsk: %u\n", ui_nsk);
> -	dev_dbg(priv->dev, "dsibclk_nsk: %u\n", dsibclk_nsk);
> +	dev_dbg(dev, "dsiclk_nsk: %u\n", dsiclk_nsk);
> +	dev_dbg(dev, "ui_nsk: %u\n", ui_nsk);
> +	dev_dbg(dev, "dsibclk_nsk: %u\n", dsibclk_nsk);
>  
>  	/* LP11 > 100us for D-PHY Rx Init */
>  	val = tc358768_ns_to_cnt(100 * 1000, dsibclk_nsk) - 1;
> -	dev_dbg(priv->dev, "LINEINITCNT: %u\n", val);
> +	dev_dbg(dev, "LINEINITCNT: %u\n", val);
>  	tc358768_write(priv, TC358768_LINEINITCNT, val);
>  
>  	/* LPTimeCnt > 50ns */
>  	val = tc358768_ns_to_cnt(50, dsibclk_nsk) - 1;
>  	lptxcnt = val;
> -	dev_dbg(priv->dev, "LPTXTIMECNT: %u\n", val);
> +	dev_dbg(dev, "LPTXTIMECNT: %u\n", val);
>  	tc358768_write(priv, TC358768_LPTXTIMECNT, val);
>  
>  	/* 38ns < TCLK_PREPARE < 95ns */
>  	val = tc358768_ns_to_cnt(65, dsibclk_nsk) - 1;
> -	dev_dbg(priv->dev, "TCLK_PREPARECNT %u\n", val);
> +	dev_dbg(dev, "TCLK_PREPARECNT %u\n", val);
>  	/* TCLK_PREPARE + TCLK_ZERO > 300ns */
>  	val2 = tc358768_ns_to_cnt(300 - tc358768_to_ns(2 * ui_nsk),
>  				  dsibclk_nsk) - 2;
> -	dev_dbg(priv->dev, "TCLK_ZEROCNT %u\n", val2);
> +	dev_dbg(dev, "TCLK_ZEROCNT %u\n", val2);
>  	val |= val2 << 8;
>  	tc358768_write(priv, TC358768_TCLK_HEADERCNT, val);
>  
>  	/* TCLK_TRAIL > 60ns AND TEOT <= 105 ns + 12*UI */
>  	raw_val = tc358768_ns_to_cnt(60 + tc358768_to_ns(2 * ui_nsk), dsibclk_nsk) - 5;
>  	val = clamp(raw_val, 0, 127);
> -	dev_dbg(priv->dev, "TCLK_TRAILCNT: %u\n", val);
> +	dev_dbg(dev, "TCLK_TRAILCNT: %u\n", val);
>  	tc358768_write(priv, TC358768_TCLK_TRAILCNT, val);
>  
>  	/* 40ns + 4*UI < THS_PREPARE < 85ns + 6*UI */
>  	val = 50 + tc358768_to_ns(4 * ui_nsk);
>  	val = tc358768_ns_to_cnt(val, dsibclk_nsk) - 1;
> -	dev_dbg(priv->dev, "THS_PREPARECNT %u\n", val);
> +	dev_dbg(dev, "THS_PREPARECNT %u\n", val);
>  	/* THS_PREPARE + THS_ZERO > 145ns + 10*UI */
>  	raw_val = tc358768_ns_to_cnt(145 - tc358768_to_ns(3 * ui_nsk), dsibclk_nsk) - 10;
>  	val2 = clamp(raw_val, 0, 127);
> -	dev_dbg(priv->dev, "THS_ZEROCNT %u\n", val2);
> +	dev_dbg(dev, "THS_ZEROCNT %u\n", val2);
>  	val |= val2 << 8;
>  	tc358768_write(priv, TC358768_THS_HEADERCNT, val);
>  
>  	/* TWAKEUP > 1ms in lptxcnt steps */
>  	val = tc358768_ns_to_cnt(1020000, dsibclk_nsk);
>  	val = val / (lptxcnt + 1) - 1;
> -	dev_dbg(priv->dev, "TWAKEUP: %u\n", val);
> +	dev_dbg(dev, "TWAKEUP: %u\n", val);
>  	tc358768_write(priv, TC358768_TWAKEUP, val);
>  
>  	/* TCLK_POSTCNT > 60ns + 52*UI */
>  	val = tc358768_ns_to_cnt(60 + tc358768_to_ns(52 * ui_nsk),
>  				 dsibclk_nsk) - 3;
> -	dev_dbg(priv->dev, "TCLK_POSTCNT: %u\n", val);
> +	dev_dbg(dev, "TCLK_POSTCNT: %u\n", val);
>  	tc358768_write(priv, TC358768_TCLK_POSTCNT, val);
>  
>  	/* max(60ns + 4*UI, 8*UI) < THS_TRAILCNT < 105ns + 12*UI */
>  	raw_val = tc358768_ns_to_cnt(60 + tc358768_to_ns(18 * ui_nsk),
>  				     dsibclk_nsk) - 4;
>  	val = clamp(raw_val, 0, 15);
> -	dev_dbg(priv->dev, "THS_TRAILCNT: %u\n", val);
> +	dev_dbg(dev, "THS_TRAILCNT: %u\n", val);
>  	tc358768_write(priv, TC358768_THS_TRAILCNT, val);
>  
>  	val = BIT(0);
> @@ -805,10 +806,10 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>  	/* TXTAGOCNT[26:16] RXTASURECNT[10:0] */
>  	val = tc358768_to_ns((lptxcnt + 1) * dsibclk_nsk * 4);
>  	val = tc358768_ns_to_cnt(val, dsibclk_nsk) / 4 - 1;
> -	dev_dbg(priv->dev, "TXTAGOCNT: %u\n", val);
> +	dev_dbg(dev, "TXTAGOCNT: %u\n", val);
>  	val2 = tc358768_ns_to_cnt(tc358768_to_ns((lptxcnt + 1) * dsibclk_nsk),
>  				  dsibclk_nsk) - 2;
> -	dev_dbg(priv->dev, "RXTASURECNT: %u\n", val2);
> +	dev_dbg(dev, "RXTASURECNT: %u\n", val2);
>  	val = val << 16 | val2;
>  	tc358768_write(priv, TC358768_BTACNTRL1, val);
>  
> @@ -902,7 +903,7 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>  
>  	ret = tc358768_clear_error(priv);
>  	if (ret) {
> -		dev_err(priv->dev, "Bridge pre_enable failed: %d\n", ret);
> +		dev_err(dev, "Bridge pre_enable failed: %d\n", ret);
>  		tc358768_bridge_disable(bridge);
>  		tc358768_bridge_post_disable(bridge);
>  	}
> 

-- 
Péter

[PATCH 07/11] drm/bridge: tc358768: Rename dsibclk to hsbyteclk

[Tomi Valkeinen]

The Toshiba documentation talks about HSByteClk when referring to the
DSI HS byte clock, whereas the driver uses 'dsibclk' name. Also, in a
few places the driver calculates the byte clock from the DSI clock, even
if the byte clock is already available in a variable.

To align the driver with the documentation, change the 'dsibclk'
variable to 'hsbyteclk'. This also make it easier to visually separate
'dsibclk' and 'dsiclk' variables.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/tc358768.c | 48 +++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
index 3266c08d9bf1..db45b4a982c0 100644
--- a/drivers/gpu/drm/bridge/tc358768.c
+++ b/drivers/gpu/drm/bridge/tc358768.c
@@ -604,7 +604,7 @@ static int tc358768_setup_pll(struct tc358768_priv *priv,
 
 	dev_dbg(priv->dev, "PLL: refclk %lu, fbd %u, prd %u, frs %u\n",
 		clk_get_rate(priv->refclk), fbd, prd, frs);
-	dev_dbg(priv->dev, "PLL: pll_clk: %u, DSIClk %u, DSIByteClk %u\n",
+	dev_dbg(priv->dev, "PLL: pll_clk: %u, DSIClk %u, HSByteClk %u\n",
 		priv->dsiclk * 2, priv->dsiclk, priv->dsiclk / 4);
 	dev_dbg(priv->dev, "PLL: pclk %u (panel: %u)\n",
 		tc358768_pll_to_pclk(priv, priv->dsiclk * 2),
@@ -646,8 +646,8 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
 	u32 val, val2, lptxcnt, hact, data_type;
 	s32 raw_val;
 	const struct drm_display_mode *mode;
-	u32 dsibclk_nsk, dsiclk_nsk, ui_nsk;
-	u32 dsiclk, dsibclk, video_start;
+	u32 hsbyteclk_nsk, dsiclk_nsk, ui_nsk;
+	u32 dsiclk, hsbyteclk, video_start;
 	const u32 internal_delay = 40;
 	int ret, i;
 	struct videomode vm;
@@ -678,7 +678,7 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
 	drm_display_mode_to_videomode(mode, &vm);
 
 	dsiclk = priv->dsiclk;
-	dsibclk = dsiclk / 4;
+	hsbyteclk = dsiclk / 4;
 
 	/* Data Format Control Register */
 	val = BIT(2) | BIT(1) | BIT(0); /* rdswap_en | dsitx_en | txdt_en */
@@ -730,67 +730,67 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
 		tc358768_write(priv, TC358768_D0W_CNTRL + i * 4, 0x0000);
 
 	/* DSI Timings */
-	dsibclk_nsk = (u32)div_u64((u64)1000000000 * TC358768_PRECISION,
-				  dsibclk);
+	hsbyteclk_nsk = (u32)div_u64((u64)1000000000 * TC358768_PRECISION,
+				  hsbyteclk);
 	dsiclk_nsk = (u32)div_u64((u64)1000000000 * TC358768_PRECISION, dsiclk);
 	ui_nsk = dsiclk_nsk / 2;
 	dev_dbg(dev, "dsiclk_nsk: %u\n", dsiclk_nsk);
 	dev_dbg(dev, "ui_nsk: %u\n", ui_nsk);
-	dev_dbg(dev, "dsibclk_nsk: %u\n", dsibclk_nsk);
+	dev_dbg(dev, "hsbyteclk_nsk: %u\n", hsbyteclk_nsk);
 
 	/* LP11 > 100us for D-PHY Rx Init */
-	val = tc358768_ns_to_cnt(100 * 1000, dsibclk_nsk) - 1;
+	val = tc358768_ns_to_cnt(100 * 1000, hsbyteclk_nsk) - 1;
 	dev_dbg(dev, "LINEINITCNT: %u\n", val);
 	tc358768_write(priv, TC358768_LINEINITCNT, val);
 
 	/* LPTimeCnt > 50ns */
-	val = tc358768_ns_to_cnt(50, dsibclk_nsk) - 1;
+	val = tc358768_ns_to_cnt(50, hsbyteclk_nsk) - 1;
 	lptxcnt = val;
 	dev_dbg(dev, "LPTXTIMECNT: %u\n", val);
 	tc358768_write(priv, TC358768_LPTXTIMECNT, val);
 
 	/* 38ns < TCLK_PREPARE < 95ns */
-	val = tc358768_ns_to_cnt(65, dsibclk_nsk) - 1;
+	val = tc358768_ns_to_cnt(65, hsbyteclk_nsk) - 1;
 	dev_dbg(dev, "TCLK_PREPARECNT %u\n", val);
 	/* TCLK_PREPARE + TCLK_ZERO > 300ns */
 	val2 = tc358768_ns_to_cnt(300 - tc358768_to_ns(2 * ui_nsk),
-				  dsibclk_nsk) - 2;
+				  hsbyteclk_nsk) - 2;
 	dev_dbg(dev, "TCLK_ZEROCNT %u\n", val2);
 	val |= val2 << 8;
 	tc358768_write(priv, TC358768_TCLK_HEADERCNT, val);
 
 	/* TCLK_TRAIL > 60ns AND TEOT <= 105 ns + 12*UI */
-	raw_val = tc358768_ns_to_cnt(60 + tc358768_to_ns(2 * ui_nsk), dsibclk_nsk) - 5;
+	raw_val = tc358768_ns_to_cnt(60 + tc358768_to_ns(2 * ui_nsk), hsbyteclk_nsk) - 5;
 	val = clamp(raw_val, 0, 127);
 	dev_dbg(dev, "TCLK_TRAILCNT: %u\n", val);
 	tc358768_write(priv, TC358768_TCLK_TRAILCNT, val);
 
 	/* 40ns + 4*UI < THS_PREPARE < 85ns + 6*UI */
 	val = 50 + tc358768_to_ns(4 * ui_nsk);
-	val = tc358768_ns_to_cnt(val, dsibclk_nsk) - 1;
+	val = tc358768_ns_to_cnt(val, hsbyteclk_nsk) - 1;
 	dev_dbg(dev, "THS_PREPARECNT %u\n", val);
 	/* THS_PREPARE + THS_ZERO > 145ns + 10*UI */
-	raw_val = tc358768_ns_to_cnt(145 - tc358768_to_ns(3 * ui_nsk), dsibclk_nsk) - 10;
+	raw_val = tc358768_ns_to_cnt(145 - tc358768_to_ns(3 * ui_nsk), hsbyteclk_nsk) - 10;
 	val2 = clamp(raw_val, 0, 127);
 	dev_dbg(dev, "THS_ZEROCNT %u\n", val2);
 	val |= val2 << 8;
 	tc358768_write(priv, TC358768_THS_HEADERCNT, val);
 
 	/* TWAKEUP > 1ms in lptxcnt steps */
-	val = tc358768_ns_to_cnt(1020000, dsibclk_nsk);
+	val = tc358768_ns_to_cnt(1020000, hsbyteclk_nsk);
 	val = val / (lptxcnt + 1) - 1;
 	dev_dbg(dev, "TWAKEUP: %u\n", val);
 	tc358768_write(priv, TC358768_TWAKEUP, val);
 
 	/* TCLK_POSTCNT > 60ns + 52*UI */
 	val = tc358768_ns_to_cnt(60 + tc358768_to_ns(52 * ui_nsk),
-				 dsibclk_nsk) - 3;
+				 hsbyteclk_nsk) - 3;
 	dev_dbg(dev, "TCLK_POSTCNT: %u\n", val);
 	tc358768_write(priv, TC358768_TCLK_POSTCNT, val);
 
 	/* max(60ns + 4*UI, 8*UI) < THS_TRAILCNT < 105ns + 12*UI */
 	raw_val = tc358768_ns_to_cnt(60 + tc358768_to_ns(18 * ui_nsk),
-				     dsibclk_nsk) - 4;
+				     hsbyteclk_nsk) - 4;
 	val = clamp(raw_val, 0, 15);
 	dev_dbg(dev, "THS_TRAILCNT: %u\n", val);
 	tc358768_write(priv, TC358768_THS_TRAILCNT, val);
@@ -804,11 +804,11 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
 		       (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);
-	val = tc358768_ns_to_cnt(val, dsibclk_nsk) / 4 - 1;
+	val = tc358768_to_ns((lptxcnt + 1) * hsbyteclk_nsk * 4);
+	val = tc358768_ns_to_cnt(val, hsbyteclk_nsk) / 4 - 1;
 	dev_dbg(dev, "TXTAGOCNT: %u\n", val);
-	val2 = tc358768_ns_to_cnt(tc358768_to_ns((lptxcnt + 1) * dsibclk_nsk),
-				  dsibclk_nsk) - 2;
+	val2 = tc358768_ns_to_cnt(tc358768_to_ns((lptxcnt + 1) * hsbyteclk_nsk),
+				  hsbyteclk_nsk) - 2;
 	dev_dbg(dev, "RXTASURECNT: %u\n", val2);
 	val = val << 16 | val2;
 	tc358768_write(priv, TC358768_BTACNTRL1, val);
@@ -831,13 +831,13 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
 
 		/* hsw * byteclk * ndl / pclk */
 		val = (u32)div_u64(vm.hsync_len *
-				   ((u64)priv->dsiclk / 4) * priv->dsi_lanes,
+				   (u64)hsbyteclk * priv->dsi_lanes,
 				   vm.pixelclock);
 		tc358768_write(priv, TC358768_DSI_HSW, val);
 
 		/* hbp * byteclk * ndl / pclk */
 		val = (u32)div_u64(vm.hback_porch *
-				   ((u64)priv->dsiclk / 4) * priv->dsi_lanes,
+				   (u64)hsbyteclk * priv->dsi_lanes,
 				   vm.pixelclock);
 		tc358768_write(priv, TC358768_DSI_HBPR, val);
 	} else {
@@ -856,7 +856,7 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
 
 		/* (hsw + hbp) * byteclk * ndl / pclk */
 		val = (u32)div_u64((vm.hsync_len + vm.hback_porch) *
-				   ((u64)priv->dsiclk / 4) * priv->dsi_lanes,
+				   (u64)hsbyteclk * priv->dsi_lanes,
 				   vm.pixelclock);
 		tc358768_write(priv, TC358768_DSI_HSW, val);
 

-- 
2.34.1

Re: [PATCH 07/11] drm/bridge: tc358768: Rename dsibclk to hsbyteclk

[Péter Ujfalusi]

On 04/08/2023 13:44, Tomi Valkeinen wrote:
> The Toshiba documentation talks about HSByteClk when referring to the
> DSI HS byte clock, whereas the driver uses 'dsibclk' name. Also, in a
> few places the driver calculates the byte clock from the DSI clock, even
> if the byte clock is already available in a variable.

If you say so ;)
I don't have access to the documentation anymore.

> 
> To align the driver with the documentation, change the 'dsibclk'
> variable to 'hsbyteclk'. This also make it easier to visually separate
> 'dsibclk' and 'dsiclk' variables.

Reviewed-by: Peter Ujfalusi <peter.ujfalusi@gmail.com>

> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/tc358768.c | 48 +++++++++++++++++++--------------------
>  1 file changed, 24 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
> index 3266c08d9bf1..db45b4a982c0 100644
> --- a/drivers/gpu/drm/bridge/tc358768.c
> +++ b/drivers/gpu/drm/bridge/tc358768.c
> @@ -604,7 +604,7 @@ static int tc358768_setup_pll(struct tc358768_priv *priv,
>  
>  	dev_dbg(priv->dev, "PLL: refclk %lu, fbd %u, prd %u, frs %u\n",
>  		clk_get_rate(priv->refclk), fbd, prd, frs);
> -	dev_dbg(priv->dev, "PLL: pll_clk: %u, DSIClk %u, DSIByteClk %u\n",
> +	dev_dbg(priv->dev, "PLL: pll_clk: %u, DSIClk %u, HSByteClk %u\n",
>  		priv->dsiclk * 2, priv->dsiclk, priv->dsiclk / 4);
>  	dev_dbg(priv->dev, "PLL: pclk %u (panel: %u)\n",
>  		tc358768_pll_to_pclk(priv, priv->dsiclk * 2),
> @@ -646,8 +646,8 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>  	u32 val, val2, lptxcnt, hact, data_type;
>  	s32 raw_val;
>  	const struct drm_display_mode *mode;
> -	u32 dsibclk_nsk, dsiclk_nsk, ui_nsk;
> -	u32 dsiclk, dsibclk, video_start;
> +	u32 hsbyteclk_nsk, dsiclk_nsk, ui_nsk;
> +	u32 dsiclk, hsbyteclk, video_start;
>  	const u32 internal_delay = 40;
>  	int ret, i;
>  	struct videomode vm;
> @@ -678,7 +678,7 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>  	drm_display_mode_to_videomode(mode, &vm);
>  
>  	dsiclk = priv->dsiclk;
> -	dsibclk = dsiclk / 4;
> +	hsbyteclk = dsiclk / 4;
>  
>  	/* Data Format Control Register */
>  	val = BIT(2) | BIT(1) | BIT(0); /* rdswap_en | dsitx_en | txdt_en */
> @@ -730,67 +730,67 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>  		tc358768_write(priv, TC358768_D0W_CNTRL + i * 4, 0x0000);
>  
>  	/* DSI Timings */
> -	dsibclk_nsk = (u32)div_u64((u64)1000000000 * TC358768_PRECISION,
> -				  dsibclk);
> +	hsbyteclk_nsk = (u32)div_u64((u64)1000000000 * TC358768_PRECISION,
> +				  hsbyteclk);
>  	dsiclk_nsk = (u32)div_u64((u64)1000000000 * TC358768_PRECISION, dsiclk);
>  	ui_nsk = dsiclk_nsk / 2;
>  	dev_dbg(dev, "dsiclk_nsk: %u\n", dsiclk_nsk);
>  	dev_dbg(dev, "ui_nsk: %u\n", ui_nsk);
> -	dev_dbg(dev, "dsibclk_nsk: %u\n", dsibclk_nsk);
> +	dev_dbg(dev, "hsbyteclk_nsk: %u\n", hsbyteclk_nsk);
>  
>  	/* LP11 > 100us for D-PHY Rx Init */
> -	val = tc358768_ns_to_cnt(100 * 1000, dsibclk_nsk) - 1;
> +	val = tc358768_ns_to_cnt(100 * 1000, hsbyteclk_nsk) - 1;
>  	dev_dbg(dev, "LINEINITCNT: %u\n", val);
>  	tc358768_write(priv, TC358768_LINEINITCNT, val);
>  
>  	/* LPTimeCnt > 50ns */
> -	val = tc358768_ns_to_cnt(50, dsibclk_nsk) - 1;
> +	val = tc358768_ns_to_cnt(50, hsbyteclk_nsk) - 1;
>  	lptxcnt = val;
>  	dev_dbg(dev, "LPTXTIMECNT: %u\n", val);
>  	tc358768_write(priv, TC358768_LPTXTIMECNT, val);
>  
>  	/* 38ns < TCLK_PREPARE < 95ns */
> -	val = tc358768_ns_to_cnt(65, dsibclk_nsk) - 1;
> +	val = tc358768_ns_to_cnt(65, hsbyteclk_nsk) - 1;
>  	dev_dbg(dev, "TCLK_PREPARECNT %u\n", val);
>  	/* TCLK_PREPARE + TCLK_ZERO > 300ns */
>  	val2 = tc358768_ns_to_cnt(300 - tc358768_to_ns(2 * ui_nsk),
> -				  dsibclk_nsk) - 2;
> +				  hsbyteclk_nsk) - 2;
>  	dev_dbg(dev, "TCLK_ZEROCNT %u\n", val2);
>  	val |= val2 << 8;
>  	tc358768_write(priv, TC358768_TCLK_HEADERCNT, val);
>  
>  	/* TCLK_TRAIL > 60ns AND TEOT <= 105 ns + 12*UI */
> -	raw_val = tc358768_ns_to_cnt(60 + tc358768_to_ns(2 * ui_nsk), dsibclk_nsk) - 5;
> +	raw_val = tc358768_ns_to_cnt(60 + tc358768_to_ns(2 * ui_nsk), hsbyteclk_nsk) - 5;
>  	val = clamp(raw_val, 0, 127);
>  	dev_dbg(dev, "TCLK_TRAILCNT: %u\n", val);
>  	tc358768_write(priv, TC358768_TCLK_TRAILCNT, val);
>  
>  	/* 40ns + 4*UI < THS_PREPARE < 85ns + 6*UI */
>  	val = 50 + tc358768_to_ns(4 * ui_nsk);
> -	val = tc358768_ns_to_cnt(val, dsibclk_nsk) - 1;
> +	val = tc358768_ns_to_cnt(val, hsbyteclk_nsk) - 1;
>  	dev_dbg(dev, "THS_PREPARECNT %u\n", val);
>  	/* THS_PREPARE + THS_ZERO > 145ns + 10*UI */
> -	raw_val = tc358768_ns_to_cnt(145 - tc358768_to_ns(3 * ui_nsk), dsibclk_nsk) - 10;
> +	raw_val = tc358768_ns_to_cnt(145 - tc358768_to_ns(3 * ui_nsk), hsbyteclk_nsk) - 10;
>  	val2 = clamp(raw_val, 0, 127);
>  	dev_dbg(dev, "THS_ZEROCNT %u\n", val2);
>  	val |= val2 << 8;
>  	tc358768_write(priv, TC358768_THS_HEADERCNT, val);
>  
>  	/* TWAKEUP > 1ms in lptxcnt steps */
> -	val = tc358768_ns_to_cnt(1020000, dsibclk_nsk);
> +	val = tc358768_ns_to_cnt(1020000, hsbyteclk_nsk);
>  	val = val / (lptxcnt + 1) - 1;
>  	dev_dbg(dev, "TWAKEUP: %u\n", val);
>  	tc358768_write(priv, TC358768_TWAKEUP, val);
>  
>  	/* TCLK_POSTCNT > 60ns + 52*UI */
>  	val = tc358768_ns_to_cnt(60 + tc358768_to_ns(52 * ui_nsk),
> -				 dsibclk_nsk) - 3;
> +				 hsbyteclk_nsk) - 3;
>  	dev_dbg(dev, "TCLK_POSTCNT: %u\n", val);
>  	tc358768_write(priv, TC358768_TCLK_POSTCNT, val);
>  
>  	/* max(60ns + 4*UI, 8*UI) < THS_TRAILCNT < 105ns + 12*UI */
>  	raw_val = tc358768_ns_to_cnt(60 + tc358768_to_ns(18 * ui_nsk),
> -				     dsibclk_nsk) - 4;
> +				     hsbyteclk_nsk) - 4;
>  	val = clamp(raw_val, 0, 15);
>  	dev_dbg(dev, "THS_TRAILCNT: %u\n", val);
>  	tc358768_write(priv, TC358768_THS_TRAILCNT, val);
> @@ -804,11 +804,11 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>  		       (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);
> -	val = tc358768_ns_to_cnt(val, dsibclk_nsk) / 4 - 1;
> +	val = tc358768_to_ns((lptxcnt + 1) * hsbyteclk_nsk * 4);
> +	val = tc358768_ns_to_cnt(val, hsbyteclk_nsk) / 4 - 1;
>  	dev_dbg(dev, "TXTAGOCNT: %u\n", val);
> -	val2 = tc358768_ns_to_cnt(tc358768_to_ns((lptxcnt + 1) * dsibclk_nsk),
> -				  dsibclk_nsk) - 2;
> +	val2 = tc358768_ns_to_cnt(tc358768_to_ns((lptxcnt + 1) * hsbyteclk_nsk),
> +				  hsbyteclk_nsk) - 2;
>  	dev_dbg(dev, "RXTASURECNT: %u\n", val2);
>  	val = val << 16 | val2;
>  	tc358768_write(priv, TC358768_BTACNTRL1, val);
> @@ -831,13 +831,13 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>  
>  		/* hsw * byteclk * ndl / pclk */
>  		val = (u32)div_u64(vm.hsync_len *
> -				   ((u64)priv->dsiclk / 4) * priv->dsi_lanes,
> +				   (u64)hsbyteclk * priv->dsi_lanes,
>  				   vm.pixelclock);
>  		tc358768_write(priv, TC358768_DSI_HSW, val);
>  
>  		/* hbp * byteclk * ndl / pclk */
>  		val = (u32)div_u64(vm.hback_porch *
> -				   ((u64)priv->dsiclk / 4) * priv->dsi_lanes,
> +				   (u64)hsbyteclk * priv->dsi_lanes,
>  				   vm.pixelclock);
>  		tc358768_write(priv, TC358768_DSI_HBPR, val);
>  	} else {
> @@ -856,7 +856,7 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>  
>  		/* (hsw + hbp) * byteclk * ndl / pclk */
>  		val = (u32)div_u64((vm.hsync_len + vm.hback_porch) *
> -				   ((u64)priv->dsiclk / 4) * priv->dsi_lanes,
> +				   (u64)hsbyteclk * priv->dsi_lanes,
>  				   vm.pixelclock);
>  		tc358768_write(priv, TC358768_DSI_HSW, val);
>  
> 

-- 
Péter

[PATCH 08/11] drm/bridge: tc358768: Clean up clock period code

[Tomi Valkeinen]

The driver defines TC358768_PRECISION as 1000, and uses "nsk" to refer
to clock periods. The original author does not remember where all this
came from. Effectively the driver is using picoseconds as the unit for
clock periods, yet referring to them by "nsk".

Clean this up by just saying the periods are in picoseconds.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/tc358768.c | 60 +++++++++++++++++++--------------------
 1 file changed, 29 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
index db45b4a982c0..9411b0fb471e 100644
--- a/drivers/gpu/drm/bridge/tc358768.c
+++ b/drivers/gpu/drm/bridge/tc358768.c
@@ -15,6 +15,7 @@
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
+#include <linux/units.h>
 
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_drv.h>
@@ -627,15 +628,14 @@ static int tc358768_setup_pll(struct tc358768_priv *priv,
 	return tc358768_clear_error(priv);
 }
 
-#define TC358768_PRECISION	1000
-static u32 tc358768_ns_to_cnt(u32 ns, u32 period_nsk)
+static u32 tc358768_ns_to_cnt(u32 ns, u32 period_ps)
 {
-	return (ns * TC358768_PRECISION + period_nsk) / period_nsk;
+	return (ns * 1000 + period_ps) / period_ps;
 }
 
-static u32 tc358768_to_ns(u32 nsk)
+static u32 tc358768_ps_to_ns(u32 ps)
 {
-	return (nsk / TC358768_PRECISION);
+	return ps / 1000;
 }
 
 static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
@@ -646,7 +646,7 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
 	u32 val, val2, lptxcnt, hact, data_type;
 	s32 raw_val;
 	const struct drm_display_mode *mode;
-	u32 hsbyteclk_nsk, dsiclk_nsk, ui_nsk;
+	u32 hsbyteclk_ps, dsiclk_ps, ui_ps;
 	u32 dsiclk, hsbyteclk, video_start;
 	const u32 internal_delay = 40;
 	int ret, i;
@@ -730,67 +730,65 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
 		tc358768_write(priv, TC358768_D0W_CNTRL + i * 4, 0x0000);
 
 	/* DSI Timings */
-	hsbyteclk_nsk = (u32)div_u64((u64)1000000000 * TC358768_PRECISION,
-				  hsbyteclk);
-	dsiclk_nsk = (u32)div_u64((u64)1000000000 * TC358768_PRECISION, dsiclk);
-	ui_nsk = dsiclk_nsk / 2;
-	dev_dbg(dev, "dsiclk_nsk: %u\n", dsiclk_nsk);
-	dev_dbg(dev, "ui_nsk: %u\n", ui_nsk);
-	dev_dbg(dev, "hsbyteclk_nsk: %u\n", hsbyteclk_nsk);
+	hsbyteclk_ps = (u32)div_u64(PICO, hsbyteclk);
+	dsiclk_ps = (u32)div_u64(PICO, dsiclk);
+	ui_ps = dsiclk_ps / 2;
+	dev_dbg(dev, "dsiclk: %u ps, ui %u ps, hsbyteclk %u ps\n", dsiclk_ps,
+		ui_ps, hsbyteclk_ps);
 
 	/* LP11 > 100us for D-PHY Rx Init */
-	val = tc358768_ns_to_cnt(100 * 1000, hsbyteclk_nsk) - 1;
+	val = tc358768_ns_to_cnt(100 * 1000, hsbyteclk_ps) - 1;
 	dev_dbg(dev, "LINEINITCNT: %u\n", val);
 	tc358768_write(priv, TC358768_LINEINITCNT, val);
 
 	/* LPTimeCnt > 50ns */
-	val = tc358768_ns_to_cnt(50, hsbyteclk_nsk) - 1;
+	val = tc358768_ns_to_cnt(50, hsbyteclk_ps) - 1;
 	lptxcnt = val;
 	dev_dbg(dev, "LPTXTIMECNT: %u\n", val);
 	tc358768_write(priv, TC358768_LPTXTIMECNT, val);
 
 	/* 38ns < TCLK_PREPARE < 95ns */
-	val = tc358768_ns_to_cnt(65, hsbyteclk_nsk) - 1;
+	val = tc358768_ns_to_cnt(65, hsbyteclk_ps) - 1;
 	dev_dbg(dev, "TCLK_PREPARECNT %u\n", val);
 	/* TCLK_PREPARE + TCLK_ZERO > 300ns */
-	val2 = tc358768_ns_to_cnt(300 - tc358768_to_ns(2 * ui_nsk),
-				  hsbyteclk_nsk) - 2;
+	val2 = tc358768_ns_to_cnt(300 - tc358768_ps_to_ns(2 * ui_ps),
+				  hsbyteclk_ps) - 2;
 	dev_dbg(dev, "TCLK_ZEROCNT %u\n", val2);
 	val |= val2 << 8;
 	tc358768_write(priv, TC358768_TCLK_HEADERCNT, val);
 
 	/* TCLK_TRAIL > 60ns AND TEOT <= 105 ns + 12*UI */
-	raw_val = tc358768_ns_to_cnt(60 + tc358768_to_ns(2 * ui_nsk), hsbyteclk_nsk) - 5;
+	raw_val = tc358768_ns_to_cnt(60 + tc358768_ps_to_ns(2 * ui_ps), hsbyteclk_ps) - 5;
 	val = clamp(raw_val, 0, 127);
 	dev_dbg(dev, "TCLK_TRAILCNT: %u\n", val);
 	tc358768_write(priv, TC358768_TCLK_TRAILCNT, val);
 
 	/* 40ns + 4*UI < THS_PREPARE < 85ns + 6*UI */
-	val = 50 + tc358768_to_ns(4 * ui_nsk);
-	val = tc358768_ns_to_cnt(val, hsbyteclk_nsk) - 1;
+	val = 50 + tc358768_ps_to_ns(4 * ui_ps);
+	val = tc358768_ns_to_cnt(val, hsbyteclk_ps) - 1;
 	dev_dbg(dev, "THS_PREPARECNT %u\n", val);
 	/* THS_PREPARE + THS_ZERO > 145ns + 10*UI */
-	raw_val = tc358768_ns_to_cnt(145 - tc358768_to_ns(3 * ui_nsk), hsbyteclk_nsk) - 10;
+	raw_val = tc358768_ns_to_cnt(145 - tc358768_ps_to_ns(3 * ui_ps), hsbyteclk_ps) - 10;
 	val2 = clamp(raw_val, 0, 127);
 	dev_dbg(dev, "THS_ZEROCNT %u\n", val2);
 	val |= val2 << 8;
 	tc358768_write(priv, TC358768_THS_HEADERCNT, val);
 
 	/* TWAKEUP > 1ms in lptxcnt steps */
-	val = tc358768_ns_to_cnt(1020000, hsbyteclk_nsk);
+	val = tc358768_ns_to_cnt(1020000, hsbyteclk_ps);
 	val = val / (lptxcnt + 1) - 1;
 	dev_dbg(dev, "TWAKEUP: %u\n", val);
 	tc358768_write(priv, TC358768_TWAKEUP, val);
 
 	/* TCLK_POSTCNT > 60ns + 52*UI */
-	val = tc358768_ns_to_cnt(60 + tc358768_to_ns(52 * ui_nsk),
-				 hsbyteclk_nsk) - 3;
+	val = tc358768_ns_to_cnt(60 + tc358768_ps_to_ns(52 * ui_ps),
+				 hsbyteclk_ps) - 3;
 	dev_dbg(dev, "TCLK_POSTCNT: %u\n", val);
 	tc358768_write(priv, TC358768_TCLK_POSTCNT, val);
 
 	/* max(60ns + 4*UI, 8*UI) < THS_TRAILCNT < 105ns + 12*UI */
-	raw_val = tc358768_ns_to_cnt(60 + tc358768_to_ns(18 * ui_nsk),
-				     hsbyteclk_nsk) - 4;
+	raw_val = tc358768_ns_to_cnt(60 + tc358768_ps_to_ns(18 * ui_ps),
+				     hsbyteclk_ps) - 4;
 	val = clamp(raw_val, 0, 15);
 	dev_dbg(dev, "THS_TRAILCNT: %u\n", val);
 	tc358768_write(priv, TC358768_THS_TRAILCNT, val);
@@ -804,11 +802,11 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
 		       (mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) ? 0 : BIT(0));
 
 	/* TXTAGOCNT[26:16] RXTASURECNT[10:0] */
-	val = tc358768_to_ns((lptxcnt + 1) * hsbyteclk_nsk * 4);
-	val = tc358768_ns_to_cnt(val, hsbyteclk_nsk) / 4 - 1;
+	val = tc358768_ps_to_ns((lptxcnt + 1) * hsbyteclk_ps * 4);
+	val = tc358768_ns_to_cnt(val, hsbyteclk_ps) / 4 - 1;
 	dev_dbg(dev, "TXTAGOCNT: %u\n", val);
-	val2 = tc358768_ns_to_cnt(tc358768_to_ns((lptxcnt + 1) * hsbyteclk_nsk),
-				  hsbyteclk_nsk) - 2;
+	val2 = tc358768_ns_to_cnt(tc358768_ps_to_ns((lptxcnt + 1) * hsbyteclk_ps),
+				  hsbyteclk_ps) - 2;
 	dev_dbg(dev, "RXTASURECNT: %u\n", val2);
 	val = val << 16 | val2;
 	tc358768_write(priv, TC358768_BTACNTRL1, val);

-- 
2.34.1

Re: [PATCH 08/11] drm/bridge: tc358768: Clean up clock period code

[Péter Ujfalusi]

On 04/08/2023 13:44, Tomi Valkeinen wrote:
> The driver defines TC358768_PRECISION as 1000, and uses "nsk" to refer
> to clock periods. The original author does not remember where all this
> came from.

I can confirm this!

> Effectively the driver is using picoseconds as the unit for
> clock periods, yet referring to them by "nsk".
> 
> Clean this up by just saying the periods are in picoseconds.

Thanks,

Reviewed-by: Peter Ujfalusi <peter.ujfalusi@gmail.com>

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/tc358768.c | 60 +++++++++++++++++++--------------------
>  1 file changed, 29 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
> index db45b4a982c0..9411b0fb471e 100644
> --- a/drivers/gpu/drm/bridge/tc358768.c
> +++ b/drivers/gpu/drm/bridge/tc358768.c
> @@ -15,6 +15,7 @@
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
> +#include <linux/units.h>
>  
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_drv.h>
> @@ -627,15 +628,14 @@ static int tc358768_setup_pll(struct tc358768_priv *priv,
>  	return tc358768_clear_error(priv);
>  }
>  
> -#define TC358768_PRECISION	1000
> -static u32 tc358768_ns_to_cnt(u32 ns, u32 period_nsk)
> +static u32 tc358768_ns_to_cnt(u32 ns, u32 period_ps)
>  {
> -	return (ns * TC358768_PRECISION + period_nsk) / period_nsk;
> +	return (ns * 1000 + period_ps) / period_ps;
>  }
>  
> -static u32 tc358768_to_ns(u32 nsk)
> +static u32 tc358768_ps_to_ns(u32 ps)
>  {
> -	return (nsk / TC358768_PRECISION);
> +	return ps / 1000;
>  }
>  
>  static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
> @@ -646,7 +646,7 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>  	u32 val, val2, lptxcnt, hact, data_type;
>  	s32 raw_val;
>  	const struct drm_display_mode *mode;
> -	u32 hsbyteclk_nsk, dsiclk_nsk, ui_nsk;
> +	u32 hsbyteclk_ps, dsiclk_ps, ui_ps;
>  	u32 dsiclk, hsbyteclk, video_start;
>  	const u32 internal_delay = 40;
>  	int ret, i;
> @@ -730,67 +730,65 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>  		tc358768_write(priv, TC358768_D0W_CNTRL + i * 4, 0x0000);
>  
>  	/* DSI Timings */
> -	hsbyteclk_nsk = (u32)div_u64((u64)1000000000 * TC358768_PRECISION,
> -				  hsbyteclk);
> -	dsiclk_nsk = (u32)div_u64((u64)1000000000 * TC358768_PRECISION, dsiclk);
> -	ui_nsk = dsiclk_nsk / 2;
> -	dev_dbg(dev, "dsiclk_nsk: %u\n", dsiclk_nsk);
> -	dev_dbg(dev, "ui_nsk: %u\n", ui_nsk);
> -	dev_dbg(dev, "hsbyteclk_nsk: %u\n", hsbyteclk_nsk);
> +	hsbyteclk_ps = (u32)div_u64(PICO, hsbyteclk);
> +	dsiclk_ps = (u32)div_u64(PICO, dsiclk);
> +	ui_ps = dsiclk_ps / 2;
> +	dev_dbg(dev, "dsiclk: %u ps, ui %u ps, hsbyteclk %u ps\n", dsiclk_ps,
> +		ui_ps, hsbyteclk_ps);
>  
>  	/* LP11 > 100us for D-PHY Rx Init */
> -	val = tc358768_ns_to_cnt(100 * 1000, hsbyteclk_nsk) - 1;
> +	val = tc358768_ns_to_cnt(100 * 1000, hsbyteclk_ps) - 1;
>  	dev_dbg(dev, "LINEINITCNT: %u\n", val);
>  	tc358768_write(priv, TC358768_LINEINITCNT, val);
>  
>  	/* LPTimeCnt > 50ns */
> -	val = tc358768_ns_to_cnt(50, hsbyteclk_nsk) - 1;
> +	val = tc358768_ns_to_cnt(50, hsbyteclk_ps) - 1;
>  	lptxcnt = val;
>  	dev_dbg(dev, "LPTXTIMECNT: %u\n", val);
>  	tc358768_write(priv, TC358768_LPTXTIMECNT, val);
>  
>  	/* 38ns < TCLK_PREPARE < 95ns */
> -	val = tc358768_ns_to_cnt(65, hsbyteclk_nsk) - 1;
> +	val = tc358768_ns_to_cnt(65, hsbyteclk_ps) - 1;
>  	dev_dbg(dev, "TCLK_PREPARECNT %u\n", val);
>  	/* TCLK_PREPARE + TCLK_ZERO > 300ns */
> -	val2 = tc358768_ns_to_cnt(300 - tc358768_to_ns(2 * ui_nsk),
> -				  hsbyteclk_nsk) - 2;
> +	val2 = tc358768_ns_to_cnt(300 - tc358768_ps_to_ns(2 * ui_ps),
> +				  hsbyteclk_ps) - 2;
>  	dev_dbg(dev, "TCLK_ZEROCNT %u\n", val2);
>  	val |= val2 << 8;
>  	tc358768_write(priv, TC358768_TCLK_HEADERCNT, val);
>  
>  	/* TCLK_TRAIL > 60ns AND TEOT <= 105 ns + 12*UI */
> -	raw_val = tc358768_ns_to_cnt(60 + tc358768_to_ns(2 * ui_nsk), hsbyteclk_nsk) - 5;
> +	raw_val = tc358768_ns_to_cnt(60 + tc358768_ps_to_ns(2 * ui_ps), hsbyteclk_ps) - 5;
>  	val = clamp(raw_val, 0, 127);
>  	dev_dbg(dev, "TCLK_TRAILCNT: %u\n", val);
>  	tc358768_write(priv, TC358768_TCLK_TRAILCNT, val);
>  
>  	/* 40ns + 4*UI < THS_PREPARE < 85ns + 6*UI */
> -	val = 50 + tc358768_to_ns(4 * ui_nsk);
> -	val = tc358768_ns_to_cnt(val, hsbyteclk_nsk) - 1;
> +	val = 50 + tc358768_ps_to_ns(4 * ui_ps);
> +	val = tc358768_ns_to_cnt(val, hsbyteclk_ps) - 1;
>  	dev_dbg(dev, "THS_PREPARECNT %u\n", val);
>  	/* THS_PREPARE + THS_ZERO > 145ns + 10*UI */
> -	raw_val = tc358768_ns_to_cnt(145 - tc358768_to_ns(3 * ui_nsk), hsbyteclk_nsk) - 10;
> +	raw_val = tc358768_ns_to_cnt(145 - tc358768_ps_to_ns(3 * ui_ps), hsbyteclk_ps) - 10;
>  	val2 = clamp(raw_val, 0, 127);
>  	dev_dbg(dev, "THS_ZEROCNT %u\n", val2);
>  	val |= val2 << 8;
>  	tc358768_write(priv, TC358768_THS_HEADERCNT, val);
>  
>  	/* TWAKEUP > 1ms in lptxcnt steps */
> -	val = tc358768_ns_to_cnt(1020000, hsbyteclk_nsk);
> +	val = tc358768_ns_to_cnt(1020000, hsbyteclk_ps);
>  	val = val / (lptxcnt + 1) - 1;
>  	dev_dbg(dev, "TWAKEUP: %u\n", val);
>  	tc358768_write(priv, TC358768_TWAKEUP, val);
>  
>  	/* TCLK_POSTCNT > 60ns + 52*UI */
> -	val = tc358768_ns_to_cnt(60 + tc358768_to_ns(52 * ui_nsk),
> -				 hsbyteclk_nsk) - 3;
> +	val = tc358768_ns_to_cnt(60 + tc358768_ps_to_ns(52 * ui_ps),
> +				 hsbyteclk_ps) - 3;
>  	dev_dbg(dev, "TCLK_POSTCNT: %u\n", val);
>  	tc358768_write(priv, TC358768_TCLK_POSTCNT, val);
>  
>  	/* max(60ns + 4*UI, 8*UI) < THS_TRAILCNT < 105ns + 12*UI */
> -	raw_val = tc358768_ns_to_cnt(60 + tc358768_to_ns(18 * ui_nsk),
> -				     hsbyteclk_nsk) - 4;
> +	raw_val = tc358768_ns_to_cnt(60 + tc358768_ps_to_ns(18 * ui_ps),
> +				     hsbyteclk_ps) - 4;
>  	val = clamp(raw_val, 0, 15);
>  	dev_dbg(dev, "THS_TRAILCNT: %u\n", val);
>  	tc358768_write(priv, TC358768_THS_TRAILCNT, val);
> @@ -804,11 +802,11 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>  		       (mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) ? 0 : BIT(0));
>  
>  	/* TXTAGOCNT[26:16] RXTASURECNT[10:0] */
> -	val = tc358768_to_ns((lptxcnt + 1) * hsbyteclk_nsk * 4);
> -	val = tc358768_ns_to_cnt(val, hsbyteclk_nsk) / 4 - 1;
> +	val = tc358768_ps_to_ns((lptxcnt + 1) * hsbyteclk_ps * 4);
> +	val = tc358768_ns_to_cnt(val, hsbyteclk_ps) / 4 - 1;
>  	dev_dbg(dev, "TXTAGOCNT: %u\n", val);
> -	val2 = tc358768_ns_to_cnt(tc358768_to_ns((lptxcnt + 1) * hsbyteclk_nsk),
> -				  hsbyteclk_nsk) - 2;
> +	val2 = tc358768_ns_to_cnt(tc358768_ps_to_ns((lptxcnt + 1) * hsbyteclk_ps),
> +				  hsbyteclk_ps) - 2;
>  	dev_dbg(dev, "RXTASURECNT: %u\n", val2);
>  	val = val << 16 | val2;
>  	tc358768_write(priv, TC358768_BTACNTRL1, val);
> 

-- 
Péter

[PATCH 09/11] drm/bridge: tc358768: Fix tc358768_ns_to_cnt()

[Tomi Valkeinen]

The tc358768_ns_to_cnt() is, most likely, supposed to do a div-round-up
operation, but it misses subtracting one from the dividend.

Fix this by just using DIV_ROUND_UP().

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

diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
index 9411b0fb471e..dc2241c18dde 100644
--- a/drivers/gpu/drm/bridge/tc358768.c
+++ b/drivers/gpu/drm/bridge/tc358768.c
@@ -630,7 +630,7 @@ static int tc358768_setup_pll(struct tc358768_priv *priv,
 
 static u32 tc358768_ns_to_cnt(u32 ns, u32 period_ps)
 {
-	return (ns * 1000 + period_ps) / period_ps;
+	return DIV_ROUND_UP(ns * 1000, period_ps);
 }
 
 static u32 tc358768_ps_to_ns(u32 ps)

-- 
2.34.1

Re: [PATCH 09/11] drm/bridge: tc358768: Fix tc358768_ns_to_cnt()

[Péter Ujfalusi]

On 04/08/2023 13:44, Tomi Valkeinen wrote:
> The tc358768_ns_to_cnt() is, most likely, supposed to do a div-round-up
> operation, but it misses subtracting one from the dividend.
> 
> Fix this by just using DIV_ROUND_UP().

Reviewed-by: Peter Ujfalusi <peter.ujfalusi@gmail.com>

> Fixes: ff1ca6397b1d ("drm/bridge: Add tc358768 driver")
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/tc358768.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
> index 9411b0fb471e..dc2241c18dde 100644
> --- a/drivers/gpu/drm/bridge/tc358768.c
> +++ b/drivers/gpu/drm/bridge/tc358768.c
> @@ -630,7 +630,7 @@ static int tc358768_setup_pll(struct tc358768_priv *priv,
>  
>  static u32 tc358768_ns_to_cnt(u32 ns, u32 period_ps)
>  {
> -	return (ns * 1000 + period_ps) / period_ps;
> +	return DIV_ROUND_UP(ns * 1000, period_ps);
>  }
>  
>  static u32 tc358768_ps_to_ns(u32 ps)
> 

-- 
Péter

[PATCH 10/11] drm/bridge: tc358768: Attempt to fix DSI horizontal timings

[Tomi Valkeinen]

The DSI horizontal timing calculations done by the driver seem to often
lead to underflows or overflows, depending on the videomode.

There are two main things the current driver doesn't seem to get right:
DSI HSW and HFP, and VSDly. However, even following Toshiba's
documentation it seems we don't always get a working display.

This patch attempts to fix the horizontal timings for DSI event mode, and
on a system with a DSI->HDMI encoder, a lot of standard HDMI modes now
seem to work. The work relies on Toshiba's documentation, but also quite
a bit on empirical testing.

This also adds timing related debug prints to make it easier to improve
on this later.

The DSI pulse mode has only been tested with a fixed-resolution panel,
which limits the testing of different modes on DSI pulse mode. However,
as the VSDly calculation also affects pulse mode, so this might cause a
regression.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/tc358768.c | 211 +++++++++++++++++++++++++++++++++-----
 1 file changed, 183 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
index dc2241c18dde..ea19de5509ed 100644
--- a/drivers/gpu/drm/bridge/tc358768.c
+++ b/drivers/gpu/drm/bridge/tc358768.c
@@ -9,6 +9,7 @@
 #include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/kernel.h>
+#include <linux/math64.h>
 #include <linux/media-bus-format.h>
 #include <linux/minmax.h>
 #include <linux/module.h>
@@ -157,6 +158,7 @@ struct tc358768_priv {
 	u32 frs;	/* PLL Freqency range for HSCK (post divider) */
 
 	u32 dsiclk;	/* pll_clk / 2 */
+	u32 pclk;	/* incoming pclk rate */
 };
 
 static inline struct tc358768_priv *dsi_host_to_tc358768(struct mipi_dsi_host
@@ -380,6 +382,7 @@ static int tc358768_calc_pll(struct tc358768_priv *priv,
 	priv->prd = best_prd;
 	priv->frs = frs;
 	priv->dsiclk = best_pll / 2;
+	priv->pclk = mode->clock * 1000;
 
 	return 0;
 }
@@ -638,6 +641,28 @@ static u32 tc358768_ps_to_ns(u32 ps)
 	return ps / 1000;
 }
 
+static u32 tc358768_dpi_to_ns(u32 val, u32 pclk)
+{
+	return (u32)div_u64((u64)val * NANO, pclk);
+}
+
+/* Convert value in DPI pixel clock units to DSI byte count */
+static u32 tc358768_dpi_to_dsi_bytes(struct tc358768_priv *priv, u32 val)
+{
+	u64 m = (u64)val * priv->dsiclk / 4 * priv->dsi_lanes;
+	u64 n = priv->pclk;
+
+	return (u32)div_u64(m + n - 1, n);
+}
+
+static u32 tc358768_dsi_bytes_to_ns(struct tc358768_priv *priv, u32 val)
+{
+	u64 m = (u64)val * NANO;
+	u64 n = priv->dsiclk / 4 * priv->dsi_lanes;
+
+	return (u32)div_u64(m, n);
+}
+
 static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
 {
 	struct tc358768_priv *priv = bridge_to_tc358768(bridge);
@@ -647,11 +672,19 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
 	s32 raw_val;
 	const struct drm_display_mode *mode;
 	u32 hsbyteclk_ps, dsiclk_ps, ui_ps;
-	u32 dsiclk, hsbyteclk, video_start;
-	const u32 internal_delay = 40;
+	u32 dsiclk, hsbyteclk;
 	int ret, i;
 	struct videomode vm;
 	struct device *dev = priv->dev;
+	/* In pixelclock units */
+	u32 dpi_htot, dpi_data_start;
+	/* In byte units */
+	u32 dsi_dpi_htot, dsi_dpi_data_start;
+	u32 dsi_hsw, dsi_hbp, dsi_hact, dsi_hfp;
+	const u32 dsi_hss = 4; /* HSS is a short packet (4 bytes) */
+	/* In hsbyteclk units */
+	u32 dsi_vsdly;
+	const u32 internal_dly = 40;
 
 	if (mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) {
 		dev_warn_once(dev, "Non-continuous mode unimplemented, falling back to continuous\n");
@@ -686,27 +719,23 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
 	case MIPI_DSI_FMT_RGB888:
 		val |= (0x3 << 4);
 		hact = vm.hactive * 3;
-		video_start = (vm.hsync_len + vm.hback_porch) * 3;
 		data_type = MIPI_DSI_PACKED_PIXEL_STREAM_24;
 		break;
 	case MIPI_DSI_FMT_RGB666:
 		val |= (0x4 << 4);
 		hact = vm.hactive * 3;
-		video_start = (vm.hsync_len + vm.hback_porch) * 3;
 		data_type = MIPI_DSI_PACKED_PIXEL_STREAM_18;
 		break;
 
 	case MIPI_DSI_FMT_RGB666_PACKED:
 		val |= (0x4 << 4) | BIT(3);
 		hact = vm.hactive * 18 / 8;
-		video_start = (vm.hsync_len + vm.hback_porch) * 18 / 8;
 		data_type = MIPI_DSI_PIXEL_STREAM_3BYTE_18;
 		break;
 
 	case MIPI_DSI_FMT_RGB565:
 		val |= (0x5 << 4);
 		hact = vm.hactive * 2;
-		video_start = (vm.hsync_len + vm.hback_porch) * 2;
 		data_type = MIPI_DSI_PACKED_PIXEL_STREAM_16;
 		break;
 	default:
@@ -716,9 +745,150 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
 		return;
 	}
 
+	/*
+	 * There are three important things to make TC358768 work correctly,
+	 * which are not trivial to manage:
+	 *
+	 * 1. Keep the DPI line-time and the DSI line-time as close to each
+	 *    other as possible.
+	 * 2. TC358768 goes to LP mode after each line's active area. The DSI
+	 *    HFP period has to be long enough for entering and exiting LP mode.
+	 *    But it is not clear how to calculate this.
+	 * 3. VSDly (video start delay) has to be long enough to ensure that the
+	 *    DSI TX does not start transmitting util we have started receiving
+	 *    pixel data from the DPI input. It is not clear how to calculate
+	 *    this either.
+	 */
+
+	dpi_htot = vm.hactive + vm.hfront_porch + vm.hsync_len + vm.hback_porch;
+	dpi_data_start = vm.hsync_len + vm.hback_porch;
+
+	dev_dbg(dev, "dpi horiz timing (pclk): %u + %u + %u + %u = %u\n",
+		vm.hsync_len, vm.hback_porch, vm.hactive, vm.hfront_porch,
+		dpi_htot);
+
+	dev_dbg(dev, "dpi horiz timing (ns): %u + %u + %u + %u = %u\n",
+		tc358768_dpi_to_ns(vm.hsync_len, vm.pixelclock),
+		tc358768_dpi_to_ns(vm.hback_porch, vm.pixelclock),
+		tc358768_dpi_to_ns(vm.hactive, vm.pixelclock),
+		tc358768_dpi_to_ns(vm.hfront_porch, vm.pixelclock),
+		tc358768_dpi_to_ns(dpi_htot, vm.pixelclock));
+
+	dev_dbg(dev, "dpi data start (ns): %u + %u = %u\n",
+		tc358768_dpi_to_ns(vm.hsync_len, vm.pixelclock),
+		tc358768_dpi_to_ns(vm.hback_porch, vm.pixelclock),
+		tc358768_dpi_to_ns(dpi_data_start, vm.pixelclock));
+
+	dsi_dpi_htot = tc358768_dpi_to_dsi_bytes(priv, dpi_htot);
+	dsi_dpi_data_start = tc358768_dpi_to_dsi_bytes(priv, dpi_data_start);
+
+	if (dsi_dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE) {
+		dsi_hsw = tc358768_dpi_to_dsi_bytes(priv, vm.hsync_len);
+		dsi_hbp = tc358768_dpi_to_dsi_bytes(priv, vm.hback_porch);
+	} else {
+		/* HBP is included in HSW in event mode */
+		dsi_hbp = 0;
+		dsi_hsw = tc358768_dpi_to_dsi_bytes(priv,
+			vm.hsync_len + vm.hback_porch);
+
+		/*
+		 * The pixel packet includes the actual pixel data, and:
+		 * DSI packet header = 4 bytes
+		 * DCS code = 1 byte
+		 * DSI packet footer = 2 bytes
+		 */
+		dsi_hact = hact + 4 + 1 + 2;
+
+		dsi_hfp = dsi_dpi_htot - dsi_hact - dsi_hsw - dsi_hss;
+
+		/*
+		 * Here we should check if HFP is long enough for entering LP
+		 * and exiting LP, but it's not clear how to calculate that.
+		 * Instead, this is a naive algorithm that just adjusts the HFP
+		 * and HSW so that HFP is (at least) roughly 2/3 of the total
+		 * blanking time.
+		 */
+		if (dsi_hfp < (dsi_hfp + dsi_hsw + dsi_hss) * 2 / 3) {
+			u32 old_hfp = dsi_hfp;
+			u32 old_hsw = dsi_hsw;
+			u32 tot = dsi_hfp + dsi_hsw + dsi_hss;
+
+			dsi_hsw = tot / 3;
+
+			/*
+			 * Seems like sometimes HSW has to be divisible by num-lanes, but
+			 * not always...
+			 */
+			dsi_hsw = roundup(dsi_hsw, priv->dsi_lanes);
+
+			dsi_hfp = dsi_dpi_htot - dsi_hact - dsi_hsw - dsi_hss;
+
+			dev_dbg(dev,
+				"hfp too short, adjusting dsi hfp and dsi hsw from %u, %u to %u, %u\n",
+				old_hfp, old_hsw, dsi_hfp, dsi_hsw);
+		}
+
+		dev_dbg(dev,
+			"dsi horiz timing (bytes): %u, %u + %u + %u + %u = %u\n",
+			dsi_hss, dsi_hsw, dsi_hbp, dsi_hact, dsi_hfp,
+			dsi_hss + dsi_hsw + dsi_hbp + dsi_hact + dsi_hfp);
+
+		dev_dbg(dev, "dsi horiz timing (ns): %u + %u + %u + %u + %u = %u\n",
+			tc358768_dsi_bytes_to_ns(priv, dsi_hss),
+			tc358768_dsi_bytes_to_ns(priv, dsi_hsw),
+			tc358768_dsi_bytes_to_ns(priv, dsi_hbp),
+			tc358768_dsi_bytes_to_ns(priv, dsi_hact),
+			tc358768_dsi_bytes_to_ns(priv, dsi_hfp),
+			tc358768_dsi_bytes_to_ns(priv, dsi_hss + dsi_hsw + dsi_hbp + dsi_hact + dsi_hfp));
+	}
+
+	/* VSDly calculation */
+
+	/* Start with the HW internal delay */
+	dsi_vsdly = internal_dly;
+
+	/* Convert to byte units as the other variables are in byte units */
+	dsi_vsdly *= priv->dsi_lanes;
+
+	/* Do we need more delay, in addition to the internal? */
+	if (dsi_dpi_data_start > dsi_vsdly + dsi_hss + dsi_hsw + dsi_hbp) {
+		dsi_vsdly = dsi_dpi_data_start - dsi_hss - dsi_hsw - dsi_hbp;
+		dsi_vsdly = roundup(dsi_vsdly, priv->dsi_lanes);
+	}
+
+	dev_dbg(dev, "dsi data start (bytes) %u + %u + %u + %u = %u\n",
+		dsi_vsdly, dsi_hss, dsi_hsw, dsi_hbp,
+		dsi_vsdly + dsi_hss + dsi_hsw + dsi_hbp);
+
+	dev_dbg(dev, "dsi data start (ns) %u + %u + %u + %u = %u\n",
+		tc358768_dsi_bytes_to_ns(priv, dsi_vsdly),
+		tc358768_dsi_bytes_to_ns(priv, dsi_hss),
+		tc358768_dsi_bytes_to_ns(priv, dsi_hsw),
+		tc358768_dsi_bytes_to_ns(priv, dsi_hbp),
+		tc358768_dsi_bytes_to_ns(priv, dsi_vsdly + dsi_hss + dsi_hsw + dsi_hbp));
+
+	/* Convert back to hsbyteclk */
+	dsi_vsdly /= priv->dsi_lanes;
+
+	/*
+	 * The docs say that there is an internal delay of 40 cycles.
+	 * However, we get underflows if we follow that rule. If we
+	 * instead ignore the internal delay, things work. So either
+	 * the docs are wrong or the calculations are wrong.
+	 *
+	 * As a temporary fix, add the internal delay here, to counter
+	 * the subtraction when writing the register.
+	 */
+	dsi_vsdly += internal_dly;
+
+	/* Clamp to the register max */
+	if (dsi_vsdly - internal_dly > 0x3ff) {
+		dev_warn(dev, "VSDly too high, underflows likely\n");
+		dsi_vsdly = 0x3ff + internal_dly;
+	}
+
 	/* VSDly[9:0] */
-	video_start = max(video_start, internal_delay + 1) - internal_delay;
-	tc358768_write(priv, TC358768_VSDLY, video_start);
+	tc358768_write(priv, TC358768_VSDLY, dsi_vsdly - internal_dly);
 
 	tc358768_write(priv, TC358768_DATAFMT, val);
 	tc358768_write(priv, TC358768_DSITX_DT, data_type);
@@ -826,18 +996,6 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
 
 		/* vbp */
 		tc358768_write(priv, TC358768_DSI_VBPR, vm.vback_porch);
-
-		/* hsw * byteclk * ndl / pclk */
-		val = (u32)div_u64(vm.hsync_len *
-				   (u64)hsbyteclk * priv->dsi_lanes,
-				   vm.pixelclock);
-		tc358768_write(priv, TC358768_DSI_HSW, val);
-
-		/* hbp * byteclk * ndl / pclk */
-		val = (u32)div_u64(vm.hback_porch *
-				   (u64)hsbyteclk * priv->dsi_lanes,
-				   vm.pixelclock);
-		tc358768_write(priv, TC358768_DSI_HBPR, val);
 	} else {
 		/* Set event mode */
 		tc358768_write(priv, TC358768_DSI_EVENT, 1);
@@ -851,16 +1009,13 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
 
 		/* vbp (not used in event mode) */
 		tc358768_write(priv, TC358768_DSI_VBPR, 0);
+	}
 
-		/* (hsw + hbp) * byteclk * ndl / pclk */
-		val = (u32)div_u64((vm.hsync_len + vm.hback_porch) *
-				   (u64)hsbyteclk * priv->dsi_lanes,
-				   vm.pixelclock);
-		tc358768_write(priv, TC358768_DSI_HSW, val);
+	/* hsw (bytes) */
+	tc358768_write(priv, TC358768_DSI_HSW, dsi_hsw);
 
-		/* hbp (not used in event mode) */
-		tc358768_write(priv, TC358768_DSI_HBPR, 0);
-	}
+	/* hbp (bytes) */
+	tc358768_write(priv, TC358768_DSI_HBPR, dsi_hbp);
 
 	/* hact (bytes) */
 	tc358768_write(priv, TC358768_DSI_HACT, hact);

-- 
2.34.1

Re: [PATCH 10/11] drm/bridge: tc358768: Attempt to fix DSI horizontal timings

[Péter Ujfalusi]

On 04/08/2023 13:44, Tomi Valkeinen wrote:
> The DSI horizontal timing calculations done by the driver seem to often
> lead to underflows or overflows, depending on the videomode.
> 
> There are two main things the current driver doesn't seem to get right:
> DSI HSW and HFP, and VSDly. However, even following Toshiba's
> documentation it seems we don't always get a working display.
> 
> This patch attempts to fix the horizontal timings for DSI event mode, and
> on a system with a DSI->HDMI encoder, a lot of standard HDMI modes now
> seem to work. The work relies on Toshiba's documentation, but also quite
> a bit on empirical testing.
> 
> This also adds timing related debug prints to make it easier to improve
> on this later.
> 
> The DSI pulse mode has only been tested with a fixed-resolution panel,
> which limits the testing of different modes on DSI pulse mode. However,
> as the VSDly calculation also affects pulse mode, so this might cause a
> regression.

If my memory serves me right we only had this used in a sinlge, static
configuration and again, it might be mentioned in a comment somewhere?

Nice work!

Reviewed-by: Peter Ujfalusi <peter.ujfalusi@gmail.com>

> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/tc358768.c | 211 +++++++++++++++++++++++++++++++++-----
>  1 file changed, 183 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
> index dc2241c18dde..ea19de5509ed 100644
> --- a/drivers/gpu/drm/bridge/tc358768.c
> +++ b/drivers/gpu/drm/bridge/tc358768.c
> @@ -9,6 +9,7 @@
>  #include <linux/gpio/consumer.h>
>  #include <linux/i2c.h>
>  #include <linux/kernel.h>
> +#include <linux/math64.h>
>  #include <linux/media-bus-format.h>
>  #include <linux/minmax.h>
>  #include <linux/module.h>
> @@ -157,6 +158,7 @@ struct tc358768_priv {
>  	u32 frs;	/* PLL Freqency range for HSCK (post divider) */
>  
>  	u32 dsiclk;	/* pll_clk / 2 */
> +	u32 pclk;	/* incoming pclk rate */
>  };
>  
>  static inline struct tc358768_priv *dsi_host_to_tc358768(struct mipi_dsi_host
> @@ -380,6 +382,7 @@ static int tc358768_calc_pll(struct tc358768_priv *priv,
>  	priv->prd = best_prd;
>  	priv->frs = frs;
>  	priv->dsiclk = best_pll / 2;
> +	priv->pclk = mode->clock * 1000;
>  
>  	return 0;
>  }
> @@ -638,6 +641,28 @@ static u32 tc358768_ps_to_ns(u32 ps)
>  	return ps / 1000;
>  }
>  
> +static u32 tc358768_dpi_to_ns(u32 val, u32 pclk)
> +{
> +	return (u32)div_u64((u64)val * NANO, pclk);
> +}
> +
> +/* Convert value in DPI pixel clock units to DSI byte count */
> +static u32 tc358768_dpi_to_dsi_bytes(struct tc358768_priv *priv, u32 val)
> +{
> +	u64 m = (u64)val * priv->dsiclk / 4 * priv->dsi_lanes;
> +	u64 n = priv->pclk;
> +
> +	return (u32)div_u64(m + n - 1, n);
> +}
> +
> +static u32 tc358768_dsi_bytes_to_ns(struct tc358768_priv *priv, u32 val)
> +{
> +	u64 m = (u64)val * NANO;
> +	u64 n = priv->dsiclk / 4 * priv->dsi_lanes;
> +
> +	return (u32)div_u64(m, n);
> +}
> +
>  static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>  {
>  	struct tc358768_priv *priv = bridge_to_tc358768(bridge);
> @@ -647,11 +672,19 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>  	s32 raw_val;
>  	const struct drm_display_mode *mode;
>  	u32 hsbyteclk_ps, dsiclk_ps, ui_ps;
> -	u32 dsiclk, hsbyteclk, video_start;
> -	const u32 internal_delay = 40;
> +	u32 dsiclk, hsbyteclk;
>  	int ret, i;
>  	struct videomode vm;
>  	struct device *dev = priv->dev;
> +	/* In pixelclock units */
> +	u32 dpi_htot, dpi_data_start;
> +	/* In byte units */
> +	u32 dsi_dpi_htot, dsi_dpi_data_start;
> +	u32 dsi_hsw, dsi_hbp, dsi_hact, dsi_hfp;
> +	const u32 dsi_hss = 4; /* HSS is a short packet (4 bytes) */
> +	/* In hsbyteclk units */
> +	u32 dsi_vsdly;
> +	const u32 internal_dly = 40;
>  
>  	if (mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) {
>  		dev_warn_once(dev, "Non-continuous mode unimplemented, falling back to continuous\n");
> @@ -686,27 +719,23 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>  	case MIPI_DSI_FMT_RGB888:
>  		val |= (0x3 << 4);
>  		hact = vm.hactive * 3;
> -		video_start = (vm.hsync_len + vm.hback_porch) * 3;
>  		data_type = MIPI_DSI_PACKED_PIXEL_STREAM_24;
>  		break;
>  	case MIPI_DSI_FMT_RGB666:
>  		val |= (0x4 << 4);
>  		hact = vm.hactive * 3;
> -		video_start = (vm.hsync_len + vm.hback_porch) * 3;
>  		data_type = MIPI_DSI_PACKED_PIXEL_STREAM_18;
>  		break;
>  
>  	case MIPI_DSI_FMT_RGB666_PACKED:
>  		val |= (0x4 << 4) | BIT(3);
>  		hact = vm.hactive * 18 / 8;
> -		video_start = (vm.hsync_len + vm.hback_porch) * 18 / 8;
>  		data_type = MIPI_DSI_PIXEL_STREAM_3BYTE_18;
>  		break;
>  
>  	case MIPI_DSI_FMT_RGB565:
>  		val |= (0x5 << 4);
>  		hact = vm.hactive * 2;
> -		video_start = (vm.hsync_len + vm.hback_porch) * 2;
>  		data_type = MIPI_DSI_PACKED_PIXEL_STREAM_16;
>  		break;
>  	default:
> @@ -716,9 +745,150 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>  		return;
>  	}
>  
> +	/*
> +	 * There are three important things to make TC358768 work correctly,
> +	 * which are not trivial to manage:
> +	 *
> +	 * 1. Keep the DPI line-time and the DSI line-time as close to each
> +	 *    other as possible.
> +	 * 2. TC358768 goes to LP mode after each line's active area. The DSI
> +	 *    HFP period has to be long enough for entering and exiting LP mode.
> +	 *    But it is not clear how to calculate this.
> +	 * 3. VSDly (video start delay) has to be long enough to ensure that the
> +	 *    DSI TX does not start transmitting util we have started receiving
> +	 *    pixel data from the DPI input. It is not clear how to calculate
> +	 *    this either.
> +	 */
> +
> +	dpi_htot = vm.hactive + vm.hfront_porch + vm.hsync_len + vm.hback_porch;
> +	dpi_data_start = vm.hsync_len + vm.hback_porch;
> +
> +	dev_dbg(dev, "dpi horiz timing (pclk): %u + %u + %u + %u = %u\n",
> +		vm.hsync_len, vm.hback_porch, vm.hactive, vm.hfront_porch,
> +		dpi_htot);
> +
> +	dev_dbg(dev, "dpi horiz timing (ns): %u + %u + %u + %u = %u\n",
> +		tc358768_dpi_to_ns(vm.hsync_len, vm.pixelclock),
> +		tc358768_dpi_to_ns(vm.hback_porch, vm.pixelclock),
> +		tc358768_dpi_to_ns(vm.hactive, vm.pixelclock),
> +		tc358768_dpi_to_ns(vm.hfront_porch, vm.pixelclock),
> +		tc358768_dpi_to_ns(dpi_htot, vm.pixelclock));
> +
> +	dev_dbg(dev, "dpi data start (ns): %u + %u = %u\n",
> +		tc358768_dpi_to_ns(vm.hsync_len, vm.pixelclock),
> +		tc358768_dpi_to_ns(vm.hback_porch, vm.pixelclock),
> +		tc358768_dpi_to_ns(dpi_data_start, vm.pixelclock));
> +
> +	dsi_dpi_htot = tc358768_dpi_to_dsi_bytes(priv, dpi_htot);
> +	dsi_dpi_data_start = tc358768_dpi_to_dsi_bytes(priv, dpi_data_start);
> +
> +	if (dsi_dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE) {
> +		dsi_hsw = tc358768_dpi_to_dsi_bytes(priv, vm.hsync_len);
> +		dsi_hbp = tc358768_dpi_to_dsi_bytes(priv, vm.hback_porch);
> +	} else {
> +		/* HBP is included in HSW in event mode */
> +		dsi_hbp = 0;
> +		dsi_hsw = tc358768_dpi_to_dsi_bytes(priv,
> +			vm.hsync_len + vm.hback_porch);
> +
> +		/*
> +		 * The pixel packet includes the actual pixel data, and:
> +		 * DSI packet header = 4 bytes
> +		 * DCS code = 1 byte
> +		 * DSI packet footer = 2 bytes
> +		 */
> +		dsi_hact = hact + 4 + 1 + 2;
> +
> +		dsi_hfp = dsi_dpi_htot - dsi_hact - dsi_hsw - dsi_hss;
> +
> +		/*
> +		 * Here we should check if HFP is long enough for entering LP
> +		 * and exiting LP, but it's not clear how to calculate that.
> +		 * Instead, this is a naive algorithm that just adjusts the HFP
> +		 * and HSW so that HFP is (at least) roughly 2/3 of the total
> +		 * blanking time.
> +		 */
> +		if (dsi_hfp < (dsi_hfp + dsi_hsw + dsi_hss) * 2 / 3) {
> +			u32 old_hfp = dsi_hfp;
> +			u32 old_hsw = dsi_hsw;
> +			u32 tot = dsi_hfp + dsi_hsw + dsi_hss;
> +
> +			dsi_hsw = tot / 3;
> +
> +			/*
> +			 * Seems like sometimes HSW has to be divisible by num-lanes, but
> +			 * not always...
> +			 */
> +			dsi_hsw = roundup(dsi_hsw, priv->dsi_lanes);
> +
> +			dsi_hfp = dsi_dpi_htot - dsi_hact - dsi_hsw - dsi_hss;
> +
> +			dev_dbg(dev,
> +				"hfp too short, adjusting dsi hfp and dsi hsw from %u, %u to %u, %u\n",
> +				old_hfp, old_hsw, dsi_hfp, dsi_hsw);
> +		}
> +
> +		dev_dbg(dev,
> +			"dsi horiz timing (bytes): %u, %u + %u + %u + %u = %u\n",
> +			dsi_hss, dsi_hsw, dsi_hbp, dsi_hact, dsi_hfp,
> +			dsi_hss + dsi_hsw + dsi_hbp + dsi_hact + dsi_hfp);
> +
> +		dev_dbg(dev, "dsi horiz timing (ns): %u + %u + %u + %u + %u = %u\n",
> +			tc358768_dsi_bytes_to_ns(priv, dsi_hss),
> +			tc358768_dsi_bytes_to_ns(priv, dsi_hsw),
> +			tc358768_dsi_bytes_to_ns(priv, dsi_hbp),
> +			tc358768_dsi_bytes_to_ns(priv, dsi_hact),
> +			tc358768_dsi_bytes_to_ns(priv, dsi_hfp),
> +			tc358768_dsi_bytes_to_ns(priv, dsi_hss + dsi_hsw + dsi_hbp + dsi_hact + dsi_hfp));
> +	}
> +
> +	/* VSDly calculation */
> +
> +	/* Start with the HW internal delay */
> +	dsi_vsdly = internal_dly;
> +
> +	/* Convert to byte units as the other variables are in byte units */
> +	dsi_vsdly *= priv->dsi_lanes;
> +
> +	/* Do we need more delay, in addition to the internal? */
> +	if (dsi_dpi_data_start > dsi_vsdly + dsi_hss + dsi_hsw + dsi_hbp) {
> +		dsi_vsdly = dsi_dpi_data_start - dsi_hss - dsi_hsw - dsi_hbp;
> +		dsi_vsdly = roundup(dsi_vsdly, priv->dsi_lanes);
> +	}
> +
> +	dev_dbg(dev, "dsi data start (bytes) %u + %u + %u + %u = %u\n",
> +		dsi_vsdly, dsi_hss, dsi_hsw, dsi_hbp,
> +		dsi_vsdly + dsi_hss + dsi_hsw + dsi_hbp);
> +
> +	dev_dbg(dev, "dsi data start (ns) %u + %u + %u + %u = %u\n",
> +		tc358768_dsi_bytes_to_ns(priv, dsi_vsdly),
> +		tc358768_dsi_bytes_to_ns(priv, dsi_hss),
> +		tc358768_dsi_bytes_to_ns(priv, dsi_hsw),
> +		tc358768_dsi_bytes_to_ns(priv, dsi_hbp),
> +		tc358768_dsi_bytes_to_ns(priv, dsi_vsdly + dsi_hss + dsi_hsw + dsi_hbp));
> +
> +	/* Convert back to hsbyteclk */
> +	dsi_vsdly /= priv->dsi_lanes;
> +
> +	/*
> +	 * The docs say that there is an internal delay of 40 cycles.
> +	 * However, we get underflows if we follow that rule. If we
> +	 * instead ignore the internal delay, things work. So either
> +	 * the docs are wrong or the calculations are wrong.
> +	 *
> +	 * As a temporary fix, add the internal delay here, to counter
> +	 * the subtraction when writing the register.
> +	 */
> +	dsi_vsdly += internal_dly;
> +
> +	/* Clamp to the register max */
> +	if (dsi_vsdly - internal_dly > 0x3ff) {
> +		dev_warn(dev, "VSDly too high, underflows likely\n");
> +		dsi_vsdly = 0x3ff + internal_dly;
> +	}
> +
>  	/* VSDly[9:0] */
> -	video_start = max(video_start, internal_delay + 1) - internal_delay;
> -	tc358768_write(priv, TC358768_VSDLY, video_start);
> +	tc358768_write(priv, TC358768_VSDLY, dsi_vsdly - internal_dly);
>  
>  	tc358768_write(priv, TC358768_DATAFMT, val);
>  	tc358768_write(priv, TC358768_DSITX_DT, data_type);
> @@ -826,18 +996,6 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>  
>  		/* vbp */
>  		tc358768_write(priv, TC358768_DSI_VBPR, vm.vback_porch);
> -
> -		/* hsw * byteclk * ndl / pclk */
> -		val = (u32)div_u64(vm.hsync_len *
> -				   (u64)hsbyteclk * priv->dsi_lanes,
> -				   vm.pixelclock);
> -		tc358768_write(priv, TC358768_DSI_HSW, val);
> -
> -		/* hbp * byteclk * ndl / pclk */
> -		val = (u32)div_u64(vm.hback_porch *
> -				   (u64)hsbyteclk * priv->dsi_lanes,
> -				   vm.pixelclock);
> -		tc358768_write(priv, TC358768_DSI_HBPR, val);
>  	} else {
>  		/* Set event mode */
>  		tc358768_write(priv, TC358768_DSI_EVENT, 1);
> @@ -851,16 +1009,13 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
>  
>  		/* vbp (not used in event mode) */
>  		tc358768_write(priv, TC358768_DSI_VBPR, 0);
> +	}
>  
> -		/* (hsw + hbp) * byteclk * ndl / pclk */
> -		val = (u32)div_u64((vm.hsync_len + vm.hback_porch) *
> -				   (u64)hsbyteclk * priv->dsi_lanes,
> -				   vm.pixelclock);
> -		tc358768_write(priv, TC358768_DSI_HSW, val);
> +	/* hsw (bytes) */
> +	tc358768_write(priv, TC358768_DSI_HSW, dsi_hsw);
>  
> -		/* hbp (not used in event mode) */
> -		tc358768_write(priv, TC358768_DSI_HBPR, 0);
> -	}
> +	/* hbp (bytes) */
> +	tc358768_write(priv, TC358768_DSI_HBPR, dsi_hbp);
>  
>  	/* hact (bytes) */
>  	tc358768_write(priv, TC358768_DSI_HACT, hact);
> 

-- 
Péter

[PATCH 11/11] drm/bridge: tc358768: Add DRM_BRIDGE_ATTACH_NO_CONNECTOR support

[Tomi Valkeinen]

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/tc358768.c | 64 +++++++++++++++++++++++++++------------
 1 file changed, 45 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
index ea19de5509ed..a567f136ddc7 100644
--- a/drivers/gpu/drm/bridge/tc358768.c
+++ b/drivers/gpu/drm/bridge/tc358768.c
@@ -131,8 +131,17 @@ static const char * const tc358768_supplies[] = {
 
 struct tc358768_dsi_output {
 	struct mipi_dsi_device *dev;
+
+	/* Legacy field if DRM_BRIDGE_ATTACH_NO_CONNECTOR is not used */
 	struct drm_panel *panel;
-	struct drm_bridge *bridge;
+
+	/*
+	 * If DRM_BRIDGE_ATTACH_NO_CONNECTOR is not used and a panel is attached
+	 * to tc358768, 'next_bridge' contains the bridge the driver created
+	 * with drm_panel_bridge_add_typed(). Otherwise 'next_bridge' contains
+	 * the next bridge the driver found.
+	 */
+	struct drm_bridge *next_bridge;
 };
 
 struct tc358768_priv {
@@ -391,8 +400,6 @@ static int tc358768_dsi_host_attach(struct mipi_dsi_host *host,
 				    struct mipi_dsi_device *dev)
 {
 	struct tc358768_priv *priv = dsi_host_to_tc358768(host);
-	struct drm_bridge *bridge;
-	struct drm_panel *panel;
 	struct device_node *ep;
 	int ret;
 
@@ -420,21 +427,7 @@ static int tc358768_dsi_host_attach(struct mipi_dsi_host *host,
 		return -ENOTSUPP;
 	}
 
-	ret = drm_of_find_panel_or_bridge(host->dev->of_node, 1, 0, &panel,
-					  &bridge);
-	if (ret)
-		return ret;
-
-	if (panel) {
-		bridge = drm_panel_bridge_add_typed(panel,
-						    DRM_MODE_CONNECTOR_DSI);
-		if (IS_ERR(bridge))
-			return PTR_ERR(bridge);
-	}
-
 	priv->output.dev = dev;
-	priv->output.bridge = bridge;
-	priv->output.panel = panel;
 
 	priv->dsi_lanes = dev->lanes;
 	priv->dsi_bpp = mipi_dsi_pixel_format_to_bpp(dev->format);
@@ -463,7 +456,7 @@ static int tc358768_dsi_host_detach(struct mipi_dsi_host *host,
 
 	drm_bridge_remove(&priv->bridge);
 	if (priv->output.panel)
-		drm_panel_bridge_remove(priv->output.bridge);
+		drm_panel_bridge_remove(priv->output.next_bridge);
 
 	return 0;
 }
@@ -544,7 +537,40 @@ static int tc358768_bridge_attach(struct drm_bridge *bridge,
 		return -ENOTSUPP;
 	}
 
-	return drm_bridge_attach(bridge->encoder, priv->output.bridge, bridge,
+	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
+		struct device_node *node;
+
+		/* Get the next bridge, connected to port@1. */
+		node = of_graph_get_remote_node(priv->dev->of_node, 1, -1);
+		if (!node)
+			return -ENODEV;
+
+		priv->output.next_bridge = of_drm_find_bridge(node);
+		of_node_put(node);
+		if (!priv->output.next_bridge)
+			return -EPROBE_DEFER;
+	} else {
+		struct drm_bridge *bridge;
+		struct drm_panel *panel;
+		int ret;
+
+		ret = drm_of_find_panel_or_bridge(priv->dev->of_node, 1, 0,
+						  &panel, &bridge);
+		if (ret)
+			return ret;
+
+		if (panel) {
+			bridge = drm_panel_bridge_add_typed(panel,
+				DRM_MODE_CONNECTOR_DSI);
+			if (IS_ERR(bridge))
+				return PTR_ERR(bridge);
+		}
+
+		priv->output.next_bridge = bridge;
+		priv->output.panel = panel;
+	}
+
+	return drm_bridge_attach(bridge->encoder, priv->output.next_bridge, bridge,
 				 flags);
 }
 

-- 
2.34.1

Re: [PATCH 11/11] drm/bridge: tc358768: Add DRM_BRIDGE_ATTACH_NO_CONNECTOR support

[Péter Ujfalusi]

On 04/08/2023 13:44, Tomi Valkeinen wrote:

I would rather have a commit message than a blank one.

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/tc358768.c | 64 +++++++++++++++++++++++++++------------
>  1 file changed, 45 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
> index ea19de5509ed..a567f136ddc7 100644
> --- a/drivers/gpu/drm/bridge/tc358768.c
> +++ b/drivers/gpu/drm/bridge/tc358768.c
> @@ -131,8 +131,17 @@ static const char * const tc358768_supplies[] = {
>  
>  struct tc358768_dsi_output {
>  	struct mipi_dsi_device *dev;
> +
> +	/* Legacy field if DRM_BRIDGE_ATTACH_NO_CONNECTOR is not used */
>  	struct drm_panel *panel;
> -	struct drm_bridge *bridge;
> +
> +	/*
> +	 * If DRM_BRIDGE_ATTACH_NO_CONNECTOR is not used and a panel is attached
> +	 * to tc358768, 'next_bridge' contains the bridge the driver created
> +	 * with drm_panel_bridge_add_typed(). Otherwise 'next_bridge' contains
> +	 * the next bridge the driver found.
> +	 */
> +	struct drm_bridge *next_bridge;

why it is better to call it next_bridge than just bridge? Is there a
prev_bridge also?

>  };
>  
>  struct tc358768_priv {
> @@ -391,8 +400,6 @@ static int tc358768_dsi_host_attach(struct mipi_dsi_host *host,
>  				    struct mipi_dsi_device *dev)
>  {
>  	struct tc358768_priv *priv = dsi_host_to_tc358768(host);
> -	struct drm_bridge *bridge;
> -	struct drm_panel *panel;
>  	struct device_node *ep;
>  	int ret;
>  
> @@ -420,21 +427,7 @@ static int tc358768_dsi_host_attach(struct mipi_dsi_host *host,
>  		return -ENOTSUPP;
>  	}
>  
> -	ret = drm_of_find_panel_or_bridge(host->dev->of_node, 1, 0, &panel,
> -					  &bridge);
> -	if (ret)
> -		return ret;
> -
> -	if (panel) {
> -		bridge = drm_panel_bridge_add_typed(panel,
> -						    DRM_MODE_CONNECTOR_DSI);
> -		if (IS_ERR(bridge))
> -			return PTR_ERR(bridge);
> -	}
> -
>  	priv->output.dev = dev;
> -	priv->output.bridge = bridge;
> -	priv->output.panel = panel;
>  
>  	priv->dsi_lanes = dev->lanes;
>  	priv->dsi_bpp = mipi_dsi_pixel_format_to_bpp(dev->format);
> @@ -463,7 +456,7 @@ static int tc358768_dsi_host_detach(struct mipi_dsi_host *host,
>  
>  	drm_bridge_remove(&priv->bridge);
>  	if (priv->output.panel)
> -		drm_panel_bridge_remove(priv->output.bridge);
> +		drm_panel_bridge_remove(priv->output.next_bridge);
>  
>  	return 0;
>  }
> @@ -544,7 +537,40 @@ static int tc358768_bridge_attach(struct drm_bridge *bridge,
>  		return -ENOTSUPP;
>  	}
>  
> -	return drm_bridge_attach(bridge->encoder, priv->output.bridge, bridge,
> +	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> +		struct device_node *node;
> +
> +		/* Get the next bridge, connected to port@1. */
> +		node = of_graph_get_remote_node(priv->dev->of_node, 1, -1);
> +		if (!node)
> +			return -ENODEV;
> +
> +		priv->output.next_bridge = of_drm_find_bridge(node);
> +		of_node_put(node);
> +		if (!priv->output.next_bridge)
> +			return -EPROBE_DEFER;
> +	} else {
> +		struct drm_bridge *bridge;
> +		struct drm_panel *panel;
> +		int ret;
> +
> +		ret = drm_of_find_panel_or_bridge(priv->dev->of_node, 1, 0,
> +						  &panel, &bridge);
> +		if (ret)
> +			return ret;
> +
> +		if (panel) {
> +			bridge = drm_panel_bridge_add_typed(panel,
> +				DRM_MODE_CONNECTOR_DSI);
> +			if (IS_ERR(bridge))
> +				return PTR_ERR(bridge);
> +		}
> +
> +		priv->output.next_bridge = bridge;
> +		priv->output.panel = panel;
> +	}
> +
> +	return drm_bridge_attach(bridge->encoder, priv->output.next_bridge, bridge,
>  				 flags);
>  }
>  
> 

-- 
Péter

Re: [PATCH 11/11] drm/bridge: tc358768: Add DRM_BRIDGE_ATTACH_NO_CONNECTOR support

[Tomi Valkeinen]

On 11/08/2023 19:44, Péter Ujfalusi wrote:
> 
> 
> On 04/08/2023 13:44, Tomi Valkeinen wrote:
> 
> I would rather have a commit message than a blank one.

Oops...

>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/gpu/drm/bridge/tc358768.c | 64 +++++++++++++++++++++++++++------------
>>   1 file changed, 45 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
>> index ea19de5509ed..a567f136ddc7 100644
>> --- a/drivers/gpu/drm/bridge/tc358768.c
>> +++ b/drivers/gpu/drm/bridge/tc358768.c
>> @@ -131,8 +131,17 @@ static const char * const tc358768_supplies[] = {
>>   
>>   struct tc358768_dsi_output {
>>   	struct mipi_dsi_device *dev;
>> +
>> +	/* Legacy field if DRM_BRIDGE_ATTACH_NO_CONNECTOR is not used */
>>   	struct drm_panel *panel;
>> -	struct drm_bridge *bridge;
>> +
>> +	/*
>> +	 * If DRM_BRIDGE_ATTACH_NO_CONNECTOR is not used and a panel is attached
>> +	 * to tc358768, 'next_bridge' contains the bridge the driver created
>> +	 * with drm_panel_bridge_add_typed(). Otherwise 'next_bridge' contains
>> +	 * the next bridge the driver found.
>> +	 */
>> +	struct drm_bridge *next_bridge;
> 
> why it is better to call it next_bridge than just bridge? Is there a
> prev_bridge also?

There is, prev bridge would be the bridge behind tc358768 in the chain. 
Bridge is tc358768. Next bridge is the following one.

Here, it's in the tc358768_dsi_output struct, so bridge is perhaps ok. I 
just wanted to be extra clear here, as I think it's often called 
next_bridge in other drivers.

  Tomi

Re: [PATCH 11/11] drm/bridge: tc358768: Add DRM_BRIDGE_ATTACH_NO_CONNECTOR support

[Maxim Schwalm]

On 04.08.23 12:44, Tomi Valkeinen wrote:
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/tc358768.c | 64 +++++++++++++++++++++++++++------------
>  1 file changed, 45 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
> index ea19de5509ed..a567f136ddc7 100644
> --- a/drivers/gpu/drm/bridge/tc358768.c
> +++ b/drivers/gpu/drm/bridge/tc358768.c
> @@ -131,8 +131,17 @@ static const char * const tc358768_supplies[] = {
>  
>  struct tc358768_dsi_output {
>  	struct mipi_dsi_device *dev;
> +
> +	/* Legacy field if DRM_BRIDGE_ATTACH_NO_CONNECTOR is not used */
>  	struct drm_panel *panel;
> -	struct drm_bridge *bridge;
> +
> +	/*
> +	 * If DRM_BRIDGE_ATTACH_NO_CONNECTOR is not used and a panel is attached
> +	 * to tc358768, 'next_bridge' contains the bridge the driver created
> +	 * with drm_panel_bridge_add_typed(). Otherwise 'next_bridge' contains
> +	 * the next bridge the driver found.
> +	 */
> +	struct drm_bridge *next_bridge;
>  };
>  
>  struct tc358768_priv {
> @@ -391,8 +400,6 @@ static int tc358768_dsi_host_attach(struct mipi_dsi_host *host,
>  				    struct mipi_dsi_device *dev)
>  {
>  	struct tc358768_priv *priv = dsi_host_to_tc358768(host);
> -	struct drm_bridge *bridge;
> -	struct drm_panel *panel;
>  	struct device_node *ep;
>  	int ret;
>  
> @@ -420,21 +427,7 @@ static int tc358768_dsi_host_attach(struct mipi_dsi_host *host,
>  		return -ENOTSUPP;
>  	}
>  
> -	ret = drm_of_find_panel_or_bridge(host->dev->of_node, 1, 0, &panel,
> -					  &bridge);
> -	if (ret)
> -		return ret;
> -
> -	if (panel) {
> -		bridge = drm_panel_bridge_add_typed(panel,
> -						    DRM_MODE_CONNECTOR_DSI);
> -		if (IS_ERR(bridge))
> -			return PTR_ERR(bridge);
> -	}
> -
>  	priv->output.dev = dev;
> -	priv->output.bridge = bridge;
> -	priv->output.panel = panel;
>  
>  	priv->dsi_lanes = dev->lanes;
>  	priv->dsi_bpp = mipi_dsi_pixel_format_to_bpp(dev->format);
> @@ -463,7 +456,7 @@ static int tc358768_dsi_host_detach(struct mipi_dsi_host *host,
>  
>  	drm_bridge_remove(&priv->bridge);
>  	if (priv->output.panel)
> -		drm_panel_bridge_remove(priv->output.bridge);
> +		drm_panel_bridge_remove(priv->output.next_bridge);
>  
>  	return 0;
>  }
> @@ -544,7 +537,40 @@ static int tc358768_bridge_attach(struct drm_bridge *bridge,
>  		return -ENOTSUPP;
>  	}
>  
> -	return drm_bridge_attach(bridge->encoder, priv->output.bridge, bridge,
> +	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> +		struct device_node *node;
> +
> +		/* Get the next bridge, connected to port@1. */
> +		node = of_graph_get_remote_node(priv->dev->of_node, 1, -1);
> +		if (!node)
> +			return -ENODEV;
> +
> +		priv->output.next_bridge = of_drm_find_bridge(node);
> +		of_node_put(node);
> +		if (!priv->output.next_bridge)
> +			return -EPROBE_DEFER;
> +	} else {
> +		struct drm_bridge *bridge;
> +		struct drm_panel *panel;
> +		int ret;
> +
> +		ret = drm_of_find_panel_or_bridge(priv->dev->of_node, 1, 0,
> +						  &panel, &bridge);
> +		if (ret)
> +			return ret;
> +
> +		if (panel) {
> +			bridge = drm_panel_bridge_add_typed(panel,
> +				DRM_MODE_CONNECTOR_DSI);
> +			if (IS_ERR(bridge))
> +				return PTR_ERR(bridge);
> +		}
> +
> +		priv->output.next_bridge = bridge;
> +		priv->output.panel = panel;
> +	}
> +
> +	return drm_bridge_attach(bridge->encoder, priv->output.next_bridge, bridge,
>  				 flags);
>  }
>  
>
This patch unfortunately breaks the display output on the Asus TF700T:

[drm:drm_bridge_attach] *ERROR* failed to attach bridge /i2c-mux/i2c@1/dsi@7 to encoder LVDS-59: -517
tegra-dc 54200000.dc: failed to initialize RGB output: -517
drm drm: failed to initialize 54200000.dc: -517
------------[ cut here ]------------
WARNING: CPU: 3 PID: 69 at lib/refcount.c:28 tegra_dc_init+0x24/0x5fc
refcount_t: underflow; use-after-free.
Modules linked in: elants_i2c panel_simple tc358768 atkbd vivaldi_fmap
CPU: 3 PID: 69 Comm: kworker/u8:6 Not tainted 6.5.0-rc2-postmarketos-grate #95
Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
Workqueue: events_unbound deferred_probe_work_func
 unwind_backtrace from show_stack+0x10/0x14
 show_stack from dump_stack_lvl+0x40/0x4c
 dump_stack_lvl from __warn+0x94/0xc0
 __warn from warn_slowpath_fmt+0x118/0x16c
 warn_slowpath_fmt from tegra_dc_init+0x24/0x5fc
 tegra_dc_init from host1x_device_init+0x84/0x15c
 host1x_device_init from host1x_drm_probe+0xd8/0x3c4
 host1x_drm_probe from really_probe+0xc8/0x2dc
 really_probe from __driver_probe_device+0x88/0x19c
 __driver_probe_device from driver_probe_device+0x30/0x104
 driver_probe_device from __device_attach_driver+0x94/0x108
 __device_attach_driver from bus_for_each_drv+0x80/0xb8
 bus_for_each_drv from __device_attach+0xa0/0x190
 __device_attach from bus_probe_device+0x88/0x8c
 bus_probe_device from deferred_probe_work_func+0x78/0xa4
 deferred_probe_work_func from process_one_work+0x208/0x420
 process_one_work from worker_thread+0x54/0x550
 worker_thread from kthread+0xdc/0xf8
 kthread from ret_from_fork+0x14/0x2c
Exception stack(0xcf9c5fb0 to 0xcf9c5ff8)
5fa0:                                     00000000 00000000 00000000 00000000
5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
---[ end trace 0000000000000000 ]---

Best regards,
Maxim

Re: [PATCH 11/11] drm/bridge: tc358768: Add DRM_BRIDGE_ATTACH_NO_CONNECTOR support

[Tomi Valkeinen]

On 13/08/2023 20:11, Maxim Schwalm wrote:
> On 04.08.23 12:44, Tomi Valkeinen wrote:
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/gpu/drm/bridge/tc358768.c | 64 +++++++++++++++++++++++++++------------
>>   1 file changed, 45 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
>> index ea19de5509ed..a567f136ddc7 100644
>> --- a/drivers/gpu/drm/bridge/tc358768.c
>> +++ b/drivers/gpu/drm/bridge/tc358768.c
>> @@ -131,8 +131,17 @@ static const char * const tc358768_supplies[] = {
>>   
>>   struct tc358768_dsi_output {
>>   	struct mipi_dsi_device *dev;
>> +
>> +	/* Legacy field if DRM_BRIDGE_ATTACH_NO_CONNECTOR is not used */
>>   	struct drm_panel *panel;
>> -	struct drm_bridge *bridge;
>> +
>> +	/*
>> +	 * If DRM_BRIDGE_ATTACH_NO_CONNECTOR is not used and a panel is attached
>> +	 * to tc358768, 'next_bridge' contains the bridge the driver created
>> +	 * with drm_panel_bridge_add_typed(). Otherwise 'next_bridge' contains
>> +	 * the next bridge the driver found.
>> +	 */
>> +	struct drm_bridge *next_bridge;
>>   };
>>   
>>   struct tc358768_priv {
>> @@ -391,8 +400,6 @@ static int tc358768_dsi_host_attach(struct mipi_dsi_host *host,
>>   				    struct mipi_dsi_device *dev)
>>   {
>>   	struct tc358768_priv *priv = dsi_host_to_tc358768(host);
>> -	struct drm_bridge *bridge;
>> -	struct drm_panel *panel;
>>   	struct device_node *ep;
>>   	int ret;
>>   
>> @@ -420,21 +427,7 @@ static int tc358768_dsi_host_attach(struct mipi_dsi_host *host,
>>   		return -ENOTSUPP;
>>   	}
>>   
>> -	ret = drm_of_find_panel_or_bridge(host->dev->of_node, 1, 0, &panel,
>> -					  &bridge);
>> -	if (ret)
>> -		return ret;
>> -
>> -	if (panel) {
>> -		bridge = drm_panel_bridge_add_typed(panel,
>> -						    DRM_MODE_CONNECTOR_DSI);
>> -		if (IS_ERR(bridge))
>> -			return PTR_ERR(bridge);
>> -	}
>> -
>>   	priv->output.dev = dev;
>> -	priv->output.bridge = bridge;
>> -	priv->output.panel = panel;
>>   
>>   	priv->dsi_lanes = dev->lanes;
>>   	priv->dsi_bpp = mipi_dsi_pixel_format_to_bpp(dev->format);
>> @@ -463,7 +456,7 @@ static int tc358768_dsi_host_detach(struct mipi_dsi_host *host,
>>   
>>   	drm_bridge_remove(&priv->bridge);
>>   	if (priv->output.panel)
>> -		drm_panel_bridge_remove(priv->output.bridge);
>> +		drm_panel_bridge_remove(priv->output.next_bridge);
>>   
>>   	return 0;
>>   }
>> @@ -544,7 +537,40 @@ static int tc358768_bridge_attach(struct drm_bridge *bridge,
>>   		return -ENOTSUPP;
>>   	}
>>   
>> -	return drm_bridge_attach(bridge->encoder, priv->output.bridge, bridge,
>> +	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
>> +		struct device_node *node;
>> +
>> +		/* Get the next bridge, connected to port@1. */
>> +		node = of_graph_get_remote_node(priv->dev->of_node, 1, -1);
>> +		if (!node)
>> +			return -ENODEV;
>> +
>> +		priv->output.next_bridge = of_drm_find_bridge(node);
>> +		of_node_put(node);
>> +		if (!priv->output.next_bridge)
>> +			return -EPROBE_DEFER;
>> +	} else {
>> +		struct drm_bridge *bridge;
>> +		struct drm_panel *panel;
>> +		int ret;
>> +
>> +		ret = drm_of_find_panel_or_bridge(priv->dev->of_node, 1, 0,
>> +						  &panel, &bridge);
>> +		if (ret)
>> +			return ret;
>> +
>> +		if (panel) {
>> +			bridge = drm_panel_bridge_add_typed(panel,
>> +				DRM_MODE_CONNECTOR_DSI);
>> +			if (IS_ERR(bridge))
>> +				return PTR_ERR(bridge);
>> +		}
>> +
>> +		priv->output.next_bridge = bridge;
>> +		priv->output.panel = panel;
>> +	}
>> +
>> +	return drm_bridge_attach(bridge->encoder, priv->output.next_bridge, bridge,
>>   				 flags);
>>   }
>>   
>>
> This patch unfortunately breaks the display output on the Asus TF700T:
> 
> [drm:drm_bridge_attach] *ERROR* failed to attach bridge /i2c-mux/i2c@1/dsi@7 to encoder LVDS-59: -517
> tegra-dc 54200000.dc: failed to initialize RGB output: -517
> drm drm: failed to initialize 54200000.dc: -517
> ------------[ cut here ]------------
> WARNING: CPU: 3 PID: 69 at lib/refcount.c:28 tegra_dc_init+0x24/0x5fc
> refcount_t: underflow; use-after-free.
> Modules linked in: elants_i2c panel_simple tc358768 atkbd vivaldi_fmap
> CPU: 3 PID: 69 Comm: kworker/u8:6 Not tainted 6.5.0-rc2-postmarketos-grate #95
> Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
> Workqueue: events_unbound deferred_probe_work_func
>   unwind_backtrace from show_stack+0x10/0x14
>   show_stack from dump_stack_lvl+0x40/0x4c
>   dump_stack_lvl from __warn+0x94/0xc0
>   __warn from warn_slowpath_fmt+0x118/0x16c
>   warn_slowpath_fmt from tegra_dc_init+0x24/0x5fc
>   tegra_dc_init from host1x_device_init+0x84/0x15c
>   host1x_device_init from host1x_drm_probe+0xd8/0x3c4
>   host1x_drm_probe from really_probe+0xc8/0x2dc
>   really_probe from __driver_probe_device+0x88/0x19c
>   __driver_probe_device from driver_probe_device+0x30/0x104
>   driver_probe_device from __device_attach_driver+0x94/0x108
>   __device_attach_driver from bus_for_each_drv+0x80/0xb8
>   bus_for_each_drv from __device_attach+0xa0/0x190
>   __device_attach from bus_probe_device+0x88/0x8c
>   bus_probe_device from deferred_probe_work_func+0x78/0xa4
>   deferred_probe_work_func from process_one_work+0x208/0x420
>   process_one_work from worker_thread+0x54/0x550
>   worker_thread from kthread+0xdc/0xf8
>   kthread from ret_from_fork+0x14/0x2c
> Exception stack(0xcf9c5fb0 to 0xcf9c5ff8)
> 5fa0:                                     00000000 00000000 00000000 00000000
> 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> ---[ end trace 0000000000000000 ]---

Sounds like the Tegra driver has issues cleaning up after getting a 
EPROBE_DEFER.

The tc358768 driver might have an issue with a setup where a panel is 
directly attached to it. I don't have such a setup, but maybe I can fake 
it just to see if the probing goes ok.

  Tomi

Re: [PATCH 11/11] drm/bridge: tc358768: Add DRM_BRIDGE_ATTACH_NO_CONNECTOR support

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 5769 bytes --]

On 13/08/2023 20:11, Maxim Schwalm wrote:
> On 04.08.23 12:44, Tomi Valkeinen wrote:
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/gpu/drm/bridge/tc358768.c | 64 +++++++++++++++++++++++++++------------
>>   1 file changed, 45 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
>> index ea19de5509ed..a567f136ddc7 100644
>> --- a/drivers/gpu/drm/bridge/tc358768.c
>> +++ b/drivers/gpu/drm/bridge/tc358768.c
>> @@ -131,8 +131,17 @@ static const char * const tc358768_supplies[] = {
>>   
>>   struct tc358768_dsi_output {
>>   	struct mipi_dsi_device *dev;
>> +
>> +	/* Legacy field if DRM_BRIDGE_ATTACH_NO_CONNECTOR is not used */
>>   	struct drm_panel *panel;
>> -	struct drm_bridge *bridge;
>> +
>> +	/*
>> +	 * If DRM_BRIDGE_ATTACH_NO_CONNECTOR is not used and a panel is attached
>> +	 * to tc358768, 'next_bridge' contains the bridge the driver created
>> +	 * with drm_panel_bridge_add_typed(). Otherwise 'next_bridge' contains
>> +	 * the next bridge the driver found.
>> +	 */
>> +	struct drm_bridge *next_bridge;
>>   };
>>   
>>   struct tc358768_priv {
>> @@ -391,8 +400,6 @@ static int tc358768_dsi_host_attach(struct mipi_dsi_host *host,
>>   				    struct mipi_dsi_device *dev)
>>   {
>>   	struct tc358768_priv *priv = dsi_host_to_tc358768(host);
>> -	struct drm_bridge *bridge;
>> -	struct drm_panel *panel;
>>   	struct device_node *ep;
>>   	int ret;
>>   
>> @@ -420,21 +427,7 @@ static int tc358768_dsi_host_attach(struct mipi_dsi_host *host,
>>   		return -ENOTSUPP;
>>   	}
>>   
>> -	ret = drm_of_find_panel_or_bridge(host->dev->of_node, 1, 0, &panel,
>> -					  &bridge);
>> -	if (ret)
>> -		return ret;
>> -
>> -	if (panel) {
>> -		bridge = drm_panel_bridge_add_typed(panel,
>> -						    DRM_MODE_CONNECTOR_DSI);
>> -		if (IS_ERR(bridge))
>> -			return PTR_ERR(bridge);
>> -	}
>> -
>>   	priv->output.dev = dev;
>> -	priv->output.bridge = bridge;
>> -	priv->output.panel = panel;
>>   
>>   	priv->dsi_lanes = dev->lanes;
>>   	priv->dsi_bpp = mipi_dsi_pixel_format_to_bpp(dev->format);
>> @@ -463,7 +456,7 @@ static int tc358768_dsi_host_detach(struct mipi_dsi_host *host,
>>   
>>   	drm_bridge_remove(&priv->bridge);
>>   	if (priv->output.panel)
>> -		drm_panel_bridge_remove(priv->output.bridge);
>> +		drm_panel_bridge_remove(priv->output.next_bridge);
>>   
>>   	return 0;
>>   }
>> @@ -544,7 +537,40 @@ static int tc358768_bridge_attach(struct drm_bridge *bridge,
>>   		return -ENOTSUPP;
>>   	}
>>   
>> -	return drm_bridge_attach(bridge->encoder, priv->output.bridge, bridge,
>> +	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
>> +		struct device_node *node;
>> +
>> +		/* Get the next bridge, connected to port@1. */
>> +		node = of_graph_get_remote_node(priv->dev->of_node, 1, -1);
>> +		if (!node)
>> +			return -ENODEV;
>> +
>> +		priv->output.next_bridge = of_drm_find_bridge(node);
>> +		of_node_put(node);
>> +		if (!priv->output.next_bridge)
>> +			return -EPROBE_DEFER;
>> +	} else {
>> +		struct drm_bridge *bridge;
>> +		struct drm_panel *panel;
>> +		int ret;
>> +
>> +		ret = drm_of_find_panel_or_bridge(priv->dev->of_node, 1, 0,
>> +						  &panel, &bridge);
>> +		if (ret)
>> +			return ret;
>> +
>> +		if (panel) {
>> +			bridge = drm_panel_bridge_add_typed(panel,
>> +				DRM_MODE_CONNECTOR_DSI);
>> +			if (IS_ERR(bridge))
>> +				return PTR_ERR(bridge);
>> +		}
>> +
>> +		priv->output.next_bridge = bridge;
>> +		priv->output.panel = panel;
>> +	}
>> +
>> +	return drm_bridge_attach(bridge->encoder, priv->output.next_bridge, bridge,
>>   				 flags);
>>   }
>>   
>>
> This patch unfortunately breaks the display output on the Asus TF700T:
> 
> [drm:drm_bridge_attach] *ERROR* failed to attach bridge /i2c-mux/i2c@1/dsi@7 to encoder LVDS-59: -517
> tegra-dc 54200000.dc: failed to initialize RGB output: -517
> drm drm: failed to initialize 54200000.dc: -517
> ------------[ cut here ]------------
> WARNING: CPU: 3 PID: 69 at lib/refcount.c:28 tegra_dc_init+0x24/0x5fc
> refcount_t: underflow; use-after-free.
> Modules linked in: elants_i2c panel_simple tc358768 atkbd vivaldi_fmap
> CPU: 3 PID: 69 Comm: kworker/u8:6 Not tainted 6.5.0-rc2-postmarketos-grate #95
> Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
> Workqueue: events_unbound deferred_probe_work_func
>   unwind_backtrace from show_stack+0x10/0x14
>   show_stack from dump_stack_lvl+0x40/0x4c
>   dump_stack_lvl from __warn+0x94/0xc0
>   __warn from warn_slowpath_fmt+0x118/0x16c
>   warn_slowpath_fmt from tegra_dc_init+0x24/0x5fc
>   tegra_dc_init from host1x_device_init+0x84/0x15c
>   host1x_device_init from host1x_drm_probe+0xd8/0x3c4
>   host1x_drm_probe from really_probe+0xc8/0x2dc
>   really_probe from __driver_probe_device+0x88/0x19c
>   __driver_probe_device from driver_probe_device+0x30/0x104
>   driver_probe_device from __device_attach_driver+0x94/0x108
>   __device_attach_driver from bus_for_each_drv+0x80/0xb8
>   bus_for_each_drv from __device_attach+0xa0/0x190
>   __device_attach from bus_probe_device+0x88/0x8c
>   bus_probe_device from deferred_probe_work_func+0x78/0xa4
>   deferred_probe_work_func from process_one_work+0x208/0x420
>   process_one_work from worker_thread+0x54/0x550
>   worker_thread from kthread+0xdc/0xf8
>   kthread from ret_from_fork+0x14/0x2c
> Exception stack(0xcf9c5fb0 to 0xcf9c5ff8)
> 5fa0:                                     00000000 00000000 00000000 00000000
> 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> ---[ end trace 0000000000000000 ]---

Hi Maxim,

Can you try the attached patch (on top of the series)? If it helps, I'll 
refresh the series with the fix.

  Tomi

[-- Attachment #2: 0001-drm-bridge-tc358768-fix-Add-DRM_BRIDGE_ATTACH_NO_CON.patch --]
[-- Type: text/x-patch, Size: 3270 bytes --]

From c13c691bd8826b978325575be9a87f577b83b86b Mon Sep 17 00:00:00 2001
From: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Date: Mon, 14 Aug 2023 13:02:23 +0300
Subject: [PATCH] drm/bridge: tc358768: fix 'Add DRM_BRIDGE_ATTACH_NO_CONNECTOR
 support'

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/tc358768.c | 56 +++++++++++++------------------
 1 file changed, 24 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
index 82ea4d9a814a..9705ce1bd028 100644
--- a/drivers/gpu/drm/bridge/tc358768.c
+++ b/drivers/gpu/drm/bridge/tc358768.c
@@ -455,8 +455,6 @@ static int tc358768_dsi_host_detach(struct mipi_dsi_host *host,
 	struct tc358768_priv *priv = dsi_host_to_tc358768(host);
 
 	drm_bridge_remove(&priv->bridge);
-	if (priv->output.panel)
-		drm_panel_bridge_remove(priv->output.next_bridge);
 
 	return 0;
 }
@@ -531,49 +529,42 @@ static int tc358768_bridge_attach(struct drm_bridge *bridge,
 				  enum drm_bridge_attach_flags flags)
 {
 	struct tc358768_priv *priv = bridge_to_tc358768(bridge);
+	struct drm_bridge *next_bridge;
+	struct drm_panel *panel;
+	int ret;
 
 	if (!drm_core_check_feature(bridge->dev, DRIVER_ATOMIC)) {
 		dev_err(priv->dev, "needs atomic updates support\n");
 		return -ENOTSUPP;
 	}
 
-	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
-		struct device_node *node;
-
-		/* Get the next bridge, connected to port@1. */
-		node = of_graph_get_remote_node(priv->dev->of_node, 1, -1);
-		if (!node)
-			return -ENODEV;
-
-		priv->output.next_bridge = of_drm_find_bridge(node);
-		of_node_put(node);
-		if (!priv->output.next_bridge)
-			return -EPROBE_DEFER;
-	} else {
-		struct drm_bridge *bridge;
-		struct drm_panel *panel;
-		int ret;
-
-		ret = drm_of_find_panel_or_bridge(priv->dev->of_node, 1, 0,
-						  &panel, &bridge);
-		if (ret)
-			return ret;
-
-		if (panel) {
-			bridge = drm_panel_bridge_add_typed(panel,
-				DRM_MODE_CONNECTOR_DSI);
-			if (IS_ERR(bridge))
-				return PTR_ERR(bridge);
-		}
+	ret = drm_of_find_panel_or_bridge(priv->dev->of_node, 1, -1, &panel,
+					  &next_bridge);
+	if (ret)
+		return ret;
 
-		priv->output.next_bridge = bridge;
-		priv->output.panel = panel;
+	if (panel) {
+		next_bridge = drm_panel_bridge_add_typed(panel,
+			DRM_MODE_CONNECTOR_DSI);
+		if (IS_ERR(next_bridge))
+			return PTR_ERR(next_bridge);
 	}
 
+	priv->output.next_bridge = next_bridge;
+	priv->output.panel = panel;
+
 	return drm_bridge_attach(bridge->encoder, priv->output.next_bridge, bridge,
 				 flags);
 }
 
+void tc358768_bridge_detach(struct drm_bridge *bridge)
+{
+	struct tc358768_priv *priv = bridge_to_tc358768(bridge);
+
+	if (priv->output.panel)
+		drm_panel_bridge_remove(priv->output.next_bridge);
+}
+
 static enum drm_mode_status
 tc358768_bridge_mode_valid(struct drm_bridge *bridge,
 			   const struct drm_display_info *info,
@@ -1156,6 +1147,7 @@ tc358768_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
 
 static const struct drm_bridge_funcs tc358768_bridge_funcs = {
 	.attach = tc358768_bridge_attach,
+	.detach = tc358768_bridge_detach,
 	.mode_valid = tc358768_bridge_mode_valid,
 	.pre_enable = tc358768_bridge_pre_enable,
 	.enable = tc358768_bridge_enable,
-- 
2.34.1

Re: [PATCH 11/11] drm/bridge: tc358768: Add DRM_BRIDGE_ATTACH_NO_CONNECTOR support

[Sam Ravnborg]

Hi Tomi,

> From c13c691bd8826b978325575be9a87f577b83b86b Mon Sep 17 00:00:00 2001
> From: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Date: Mon, 14 Aug 2023 13:02:23 +0300
> Subject: [PATCH] drm/bridge: tc358768: fix 'Add DRM_BRIDGE_ATTACH_NO_CONNECTOR
>  support'
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/tc358768.c | 56 +++++++++++++------------------
>  1 file changed, 24 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
> index 82ea4d9a814a..9705ce1bd028 100644
> --- a/drivers/gpu/drm/bridge/tc358768.c
> +++ b/drivers/gpu/drm/bridge/tc358768.c
> @@ -455,8 +455,6 @@ static int tc358768_dsi_host_detach(struct mipi_dsi_host *host,
>  	struct tc358768_priv *priv = dsi_host_to_tc358768(host);
>  
>  	drm_bridge_remove(&priv->bridge);
> -	if (priv->output.panel)
> -		drm_panel_bridge_remove(priv->output.next_bridge);
>  
>  	return 0;
>  }
> @@ -531,49 +529,42 @@ static int tc358768_bridge_attach(struct drm_bridge *bridge,
>  				  enum drm_bridge_attach_flags flags)
>  {
>  	struct tc358768_priv *priv = bridge_to_tc358768(bridge);
> +	struct drm_bridge *next_bridge;
> +	struct drm_panel *panel;
> +	int ret;
>  
>  	if (!drm_core_check_feature(bridge->dev, DRIVER_ATOMIC)) {
>  		dev_err(priv->dev, "needs atomic updates support\n");
>  		return -ENOTSUPP;
>  	}
>  
> -	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> -		struct device_node *node;
> -
> -		/* Get the next bridge, connected to port@1. */
> -		node = of_graph_get_remote_node(priv->dev->of_node, 1, -1);
> -		if (!node)
> -			return -ENODEV;
> -
> -		priv->output.next_bridge = of_drm_find_bridge(node);
> -		of_node_put(node);
> -		if (!priv->output.next_bridge)
> -			return -EPROBE_DEFER;
> -	} else {
> -		struct drm_bridge *bridge;
> -		struct drm_panel *panel;
> -		int ret;
> -
> -		ret = drm_of_find_panel_or_bridge(priv->dev->of_node, 1, 0,
> -						  &panel, &bridge);
> -		if (ret)
> -			return ret;
> -
> -		if (panel) {
> -			bridge = drm_panel_bridge_add_typed(panel,
> -				DRM_MODE_CONNECTOR_DSI);
> -			if (IS_ERR(bridge))
> -				return PTR_ERR(bridge);
> -		}
> +	ret = drm_of_find_panel_or_bridge(priv->dev->of_node, 1, -1, &panel,
> +					  &next_bridge);

I think the right way is to wrap the panel in a bridge,
so something like:

	next_bridge = devm_drm_of_get_bridge(dev, priv->dev->of_node, 1, -1)

	if (IS_ERR(next_bridge))
		return ...
	priv->output.next_bridge = next_bridge;


	Sam


> +	if (ret)
> +		return ret;
>  
> -		priv->output.next_bridge = bridge;
> -		priv->output.panel = panel;
> +	if (panel) {
> +		next_bridge = drm_panel_bridge_add_typed(panel,
> +			DRM_MODE_CONNECTOR_DSI);
> +		if (IS_ERR(next_bridge))
> +			return PTR_ERR(next_bridge);
>  	}
>  
> +	priv->output.next_bridge = next_bridge;
> +	priv->output.panel = panel;
> +
>  	return drm_bridge_attach(bridge->encoder, priv->output.next_bridge, bridge,
>  				 flags);
>  }
>  
> +void tc358768_bridge_detach(struct drm_bridge *bridge)
> +{
> +	struct tc358768_priv *priv = bridge_to_tc358768(bridge);
> +
> +	if (priv->output.panel)
> +		drm_panel_bridge_remove(priv->output.next_bridge);
> +}
> +
>  static enum drm_mode_status
>  tc358768_bridge_mode_valid(struct drm_bridge *bridge,
>  			   const struct drm_display_info *info,
> @@ -1156,6 +1147,7 @@ tc358768_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
>  
>  static const struct drm_bridge_funcs tc358768_bridge_funcs = {
>  	.attach = tc358768_bridge_attach,
> +	.detach = tc358768_bridge_detach,
>  	.mode_valid = tc358768_bridge_mode_valid,
>  	.pre_enable = tc358768_bridge_pre_enable,
>  	.enable = tc358768_bridge_enable,
> -- 
> 2.34.1
> 

Re: [PATCH 11/11] drm/bridge: tc358768: Add DRM_BRIDGE_ATTACH_NO_CONNECTOR support

[Laurent Pinchart]

On Mon, Aug 14, 2023 at 12:10:41PM +0200, Sam Ravnborg wrote:
> > From c13c691bd8826b978325575be9a87f577b83b86b Mon Sep 17 00:00:00 2001
> > From: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > Date: Mon, 14 Aug 2023 13:02:23 +0300
> > Subject: [PATCH] drm/bridge: tc358768: fix 'Add DRM_BRIDGE_ATTACH_NO_CONNECTOR
> >  support'
> > 
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > ---
> >  drivers/gpu/drm/bridge/tc358768.c | 56 +++++++++++++------------------
> >  1 file changed, 24 insertions(+), 32 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
> > index 82ea4d9a814a..9705ce1bd028 100644
> > --- a/drivers/gpu/drm/bridge/tc358768.c
> > +++ b/drivers/gpu/drm/bridge/tc358768.c
> > @@ -455,8 +455,6 @@ static int tc358768_dsi_host_detach(struct mipi_dsi_host *host,
> >  	struct tc358768_priv *priv = dsi_host_to_tc358768(host);
> >  
> >  	drm_bridge_remove(&priv->bridge);
> > -	if (priv->output.panel)
> > -		drm_panel_bridge_remove(priv->output.next_bridge);
> >  
> >  	return 0;
> >  }
> > @@ -531,49 +529,42 @@ static int tc358768_bridge_attach(struct drm_bridge *bridge,
> >  				  enum drm_bridge_attach_flags flags)
> >  {
> >  	struct tc358768_priv *priv = bridge_to_tc358768(bridge);
> > +	struct drm_bridge *next_bridge;
> > +	struct drm_panel *panel;
> > +	int ret;
> >  
> >  	if (!drm_core_check_feature(bridge->dev, DRIVER_ATOMIC)) {
> >  		dev_err(priv->dev, "needs atomic updates support\n");
> >  		return -ENOTSUPP;
> >  	}
> >  
> > -	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> > -		struct device_node *node;
> > -
> > -		/* Get the next bridge, connected to port@1. */
> > -		node = of_graph_get_remote_node(priv->dev->of_node, 1, -1);
> > -		if (!node)
> > -			return -ENODEV;
> > -
> > -		priv->output.next_bridge = of_drm_find_bridge(node);
> > -		of_node_put(node);
> > -		if (!priv->output.next_bridge)
> > -			return -EPROBE_DEFER;
> > -	} else {
> > -		struct drm_bridge *bridge;
> > -		struct drm_panel *panel;
> > -		int ret;
> > -
> > -		ret = drm_of_find_panel_or_bridge(priv->dev->of_node, 1, 0,
> > -						  &panel, &bridge);
> > -		if (ret)
> > -			return ret;
> > -
> > -		if (panel) {
> > -			bridge = drm_panel_bridge_add_typed(panel,
> > -				DRM_MODE_CONNECTOR_DSI);
> > -			if (IS_ERR(bridge))
> > -				return PTR_ERR(bridge);
> > -		}
> > +	ret = drm_of_find_panel_or_bridge(priv->dev->of_node, 1, -1, &panel,
> > +					  &next_bridge);
> 
> I think the right way is to wrap the panel in a bridge,
> so something like:
> 
> 	next_bridge = devm_drm_of_get_bridge(dev, priv->dev->of_node, 1, -1)
> 
> 	if (IS_ERR(next_bridge))
> 		return ...
> 	priv->output.next_bridge = next_bridge;

Should we at some point bite the bullet and wrap panels in bridges
directly in drm_panel.c ? That would simplify all the consumers.

> > +	if (ret)
> > +		return ret;
> >  
> > -		priv->output.next_bridge = bridge;
> > -		priv->output.panel = panel;
> > +	if (panel) {
> > +		next_bridge = drm_panel_bridge_add_typed(panel,
> > +			DRM_MODE_CONNECTOR_DSI);
> > +		if (IS_ERR(next_bridge))
> > +			return PTR_ERR(next_bridge);
> >  	}
> >  
> > +	priv->output.next_bridge = next_bridge;
> > +	priv->output.panel = panel;
> > +
> >  	return drm_bridge_attach(bridge->encoder, priv->output.next_bridge, bridge,
> >  				 flags);
> >  }
> >  
> > +void tc358768_bridge_detach(struct drm_bridge *bridge)
> > +{
> > +	struct tc358768_priv *priv = bridge_to_tc358768(bridge);
> > +
> > +	if (priv->output.panel)
> > +		drm_panel_bridge_remove(priv->output.next_bridge);
> > +}
> > +
> >  static enum drm_mode_status
> >  tc358768_bridge_mode_valid(struct drm_bridge *bridge,
> >  			   const struct drm_display_info *info,
> > @@ -1156,6 +1147,7 @@ tc358768_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
> >  
> >  static const struct drm_bridge_funcs tc358768_bridge_funcs = {
> >  	.attach = tc358768_bridge_attach,
> > +	.detach = tc358768_bridge_detach,
> >  	.mode_valid = tc358768_bridge_mode_valid,
> >  	.pre_enable = tc358768_bridge_pre_enable,
> >  	.enable = tc358768_bridge_enable,

-- 
Regards,

Laurent Pinchart

Re: [PATCH 11/11] drm/bridge: tc358768: Add DRM_BRIDGE_ATTACH_NO_CONNECTOR support

[Tomi Valkeinen]

Hi Sam,

On 14/08/2023 13:10, Sam Ravnborg wrote:
> Hi Tomi,
> 
>>  From c13c691bd8826b978325575be9a87f577b83b86b Mon Sep 17 00:00:00 2001
>> From: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> Date: Mon, 14 Aug 2023 13:02:23 +0300
>> Subject: [PATCH] drm/bridge: tc358768: fix 'Add DRM_BRIDGE_ATTACH_NO_CONNECTOR
>>   support'
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/gpu/drm/bridge/tc358768.c | 56 +++++++++++++------------------
>>   1 file changed, 24 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
>> index 82ea4d9a814a..9705ce1bd028 100644
>> --- a/drivers/gpu/drm/bridge/tc358768.c
>> +++ b/drivers/gpu/drm/bridge/tc358768.c
>> @@ -455,8 +455,6 @@ static int tc358768_dsi_host_detach(struct mipi_dsi_host *host,
>>   	struct tc358768_priv *priv = dsi_host_to_tc358768(host);
>>   
>>   	drm_bridge_remove(&priv->bridge);
>> -	if (priv->output.panel)
>> -		drm_panel_bridge_remove(priv->output.next_bridge);
>>   
>>   	return 0;
>>   }
>> @@ -531,49 +529,42 @@ static int tc358768_bridge_attach(struct drm_bridge *bridge,
>>   				  enum drm_bridge_attach_flags flags)
>>   {
>>   	struct tc358768_priv *priv = bridge_to_tc358768(bridge);
>> +	struct drm_bridge *next_bridge;
>> +	struct drm_panel *panel;
>> +	int ret;
>>   
>>   	if (!drm_core_check_feature(bridge->dev, DRIVER_ATOMIC)) {
>>   		dev_err(priv->dev, "needs atomic updates support\n");
>>   		return -ENOTSUPP;
>>   	}
>>   
>> -	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
>> -		struct device_node *node;
>> -
>> -		/* Get the next bridge, connected to port@1. */
>> -		node = of_graph_get_remote_node(priv->dev->of_node, 1, -1);
>> -		if (!node)
>> -			return -ENODEV;
>> -
>> -		priv->output.next_bridge = of_drm_find_bridge(node);
>> -		of_node_put(node);
>> -		if (!priv->output.next_bridge)
>> -			return -EPROBE_DEFER;
>> -	} else {
>> -		struct drm_bridge *bridge;
>> -		struct drm_panel *panel;
>> -		int ret;
>> -
>> -		ret = drm_of_find_panel_or_bridge(priv->dev->of_node, 1, 0,
>> -						  &panel, &bridge);
>> -		if (ret)
>> -			return ret;
>> -
>> -		if (panel) {
>> -			bridge = drm_panel_bridge_add_typed(panel,
>> -				DRM_MODE_CONNECTOR_DSI);
>> -			if (IS_ERR(bridge))
>> -				return PTR_ERR(bridge);
>> -		}
>> +	ret = drm_of_find_panel_or_bridge(priv->dev->of_node, 1, -1, &panel,
>> +					  &next_bridge);
> 
> I think the right way is to wrap the panel in a bridge,
> so something like:
> 
> 	next_bridge = devm_drm_of_get_bridge(dev, priv->dev->of_node, 1, -1)
> 
> 	if (IS_ERR(next_bridge))
> 		return ...
> 	priv->output.next_bridge = next_bridge;

I tried that, but I had trouble with the cleanup side.

In the fixup patch I attached in my reply to Maxim I used 
drm_of_find_panel_or_bridge() + drm_panel_bridge_add_typed(), and on 
bridge_detach callback I used drm_panel_bridge_remove() (if there is a 
panel). This worked for me, but it does feel like a bit too much work 
for a driver to do.

  Tomi

Re: [PATCH 11/11] drm/bridge: tc358768: Add DRM_BRIDGE_ATTACH_NO_CONNECTOR support

[Maxim Schwalm]

On 14.08.23 12:04, Tomi Valkeinen wrote:
> On 13/08/2023 20:11, Maxim Schwalm wrote:
>> On 04.08.23 12:44, Tomi Valkeinen wrote:
>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>> ---
>>>   drivers/gpu/drm/bridge/tc358768.c | 64 +++++++++++++++++++++++++++------------
>>>   1 file changed, 45 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c
>>> index ea19de5509ed..a567f136ddc7 100644
>>> --- a/drivers/gpu/drm/bridge/tc358768.c
>>> +++ b/drivers/gpu/drm/bridge/tc358768.c
>>> @@ -131,8 +131,17 @@ static const char * const tc358768_supplies[] = {
>>>   
>>>   struct tc358768_dsi_output {
>>>   	struct mipi_dsi_device *dev;
>>> +
>>> +	/* Legacy field if DRM_BRIDGE_ATTACH_NO_CONNECTOR is not used */
>>>   	struct drm_panel *panel;
>>> -	struct drm_bridge *bridge;
>>> +
>>> +	/*
>>> +	 * If DRM_BRIDGE_ATTACH_NO_CONNECTOR is not used and a panel is attached
>>> +	 * to tc358768, 'next_bridge' contains the bridge the driver created
>>> +	 * with drm_panel_bridge_add_typed(). Otherwise 'next_bridge' contains
>>> +	 * the next bridge the driver found.
>>> +	 */
>>> +	struct drm_bridge *next_bridge;
>>>   };
>>>   
>>>   struct tc358768_priv {
>>> @@ -391,8 +400,6 @@ static int tc358768_dsi_host_attach(struct mipi_dsi_host *host,
>>>   				    struct mipi_dsi_device *dev)
>>>   {
>>>   	struct tc358768_priv *priv = dsi_host_to_tc358768(host);
>>> -	struct drm_bridge *bridge;
>>> -	struct drm_panel *panel;
>>>   	struct device_node *ep;
>>>   	int ret;
>>>   
>>> @@ -420,21 +427,7 @@ static int tc358768_dsi_host_attach(struct mipi_dsi_host *host,
>>>   		return -ENOTSUPP;
>>>   	}
>>>   
>>> -	ret = drm_of_find_panel_or_bridge(host->dev->of_node, 1, 0, &panel,
>>> -					  &bridge);
>>> -	if (ret)
>>> -		return ret;
>>> -
>>> -	if (panel) {
>>> -		bridge = drm_panel_bridge_add_typed(panel,
>>> -						    DRM_MODE_CONNECTOR_DSI);
>>> -		if (IS_ERR(bridge))
>>> -			return PTR_ERR(bridge);
>>> -	}
>>> -
>>>   	priv->output.dev = dev;
>>> -	priv->output.bridge = bridge;
>>> -	priv->output.panel = panel;
>>>   
>>>   	priv->dsi_lanes = dev->lanes;
>>>   	priv->dsi_bpp = mipi_dsi_pixel_format_to_bpp(dev->format);
>>> @@ -463,7 +456,7 @@ static int tc358768_dsi_host_detach(struct mipi_dsi_host *host,
>>>   
>>>   	drm_bridge_remove(&priv->bridge);
>>>   	if (priv->output.panel)
>>> -		drm_panel_bridge_remove(priv->output.bridge);
>>> +		drm_panel_bridge_remove(priv->output.next_bridge);
>>>   
>>>   	return 0;
>>>   }
>>> @@ -544,7 +537,40 @@ static int tc358768_bridge_attach(struct drm_bridge *bridge,
>>>   		return -ENOTSUPP;
>>>   	}
>>>   
>>> -	return drm_bridge_attach(bridge->encoder, priv->output.bridge, bridge,
>>> +	if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
>>> +		struct device_node *node;
>>> +
>>> +		/* Get the next bridge, connected to port@1. */
>>> +		node = of_graph_get_remote_node(priv->dev->of_node, 1, -1);
>>> +		if (!node)
>>> +			return -ENODEV;
>>> +
>>> +		priv->output.next_bridge = of_drm_find_bridge(node);
>>> +		of_node_put(node);
>>> +		if (!priv->output.next_bridge)
>>> +			return -EPROBE_DEFER;
>>> +	} else {
>>> +		struct drm_bridge *bridge;
>>> +		struct drm_panel *panel;
>>> +		int ret;
>>> +
>>> +		ret = drm_of_find_panel_or_bridge(priv->dev->of_node, 1, 0,
>>> +						  &panel, &bridge);
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		if (panel) {
>>> +			bridge = drm_panel_bridge_add_typed(panel,
>>> +				DRM_MODE_CONNECTOR_DSI);
>>> +			if (IS_ERR(bridge))
>>> +				return PTR_ERR(bridge);
>>> +		}
>>> +
>>> +		priv->output.next_bridge = bridge;
>>> +		priv->output.panel = panel;
>>> +	}
>>> +
>>> +	return drm_bridge_attach(bridge->encoder, priv->output.next_bridge, bridge,
>>>   				 flags);
>>>   }
>>>   
>>>
>> This patch unfortunately breaks the display output on the Asus TF700T:
>>
>> [drm:drm_bridge_attach] *ERROR* failed to attach bridge /i2c-mux/i2c@1/dsi@7 to encoder LVDS-59: -517
>> tegra-dc 54200000.dc: failed to initialize RGB output: -517
>> drm drm: failed to initialize 54200000.dc: -517
>> ------------[ cut here ]------------
>> WARNING: CPU: 3 PID: 69 at lib/refcount.c:28 tegra_dc_init+0x24/0x5fc
>> refcount_t: underflow; use-after-free.
>> Modules linked in: elants_i2c panel_simple tc358768 atkbd vivaldi_fmap
>> CPU: 3 PID: 69 Comm: kworker/u8:6 Not tainted 6.5.0-rc2-postmarketos-grate #95
>> Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
>> Workqueue: events_unbound deferred_probe_work_func
>>   unwind_backtrace from show_stack+0x10/0x14
>>   show_stack from dump_stack_lvl+0x40/0x4c
>>   dump_stack_lvl from __warn+0x94/0xc0
>>   __warn from warn_slowpath_fmt+0x118/0x16c
>>   warn_slowpath_fmt from tegra_dc_init+0x24/0x5fc
>>   tegra_dc_init from host1x_device_init+0x84/0x15c
>>   host1x_device_init from host1x_drm_probe+0xd8/0x3c4
>>   host1x_drm_probe from really_probe+0xc8/0x2dc
>>   really_probe from __driver_probe_device+0x88/0x19c
>>   __driver_probe_device from driver_probe_device+0x30/0x104
>>   driver_probe_device from __device_attach_driver+0x94/0x108
>>   __device_attach_driver from bus_for_each_drv+0x80/0xb8
>>   bus_for_each_drv from __device_attach+0xa0/0x190
>>   __device_attach from bus_probe_device+0x88/0x8c
>>   bus_probe_device from deferred_probe_work_func+0x78/0xa4
>>   deferred_probe_work_func from process_one_work+0x208/0x420
>>   process_one_work from worker_thread+0x54/0x550
>>   worker_thread from kthread+0xdc/0xf8
>>   kthread from ret_from_fork+0x14/0x2c
>> Exception stack(0xcf9c5fb0 to 0xcf9c5ff8)
>> 5fa0:                                     00000000 00000000 00000000 00000000
>> 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>> 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
>> ---[ end trace 0000000000000000 ]---
> 
> Hi Maxim,
> 
> Can you try the attached patch (on top of the series)? If it helps, I'll 
> refresh the series with the fix.
> 

Thanks, Tomi! This fixes the issue.

Best regards,
Maxim