[PATCH 1/2] drm: bridge: tc358767: increase PLL lock time delay

[PATCH 1/2] drm: bridge: tc358767: increase PLL lock time delay

[Lucas Stach]

From: David Jander <david@protonic.nl>

The PLL often fails to lock with this delay. The new value was
determined by trial and error increasing the delay bit by bit
until the error did not occurr anymore even after several tries.
Then double that value was taken as the minimum delay to be safe.

Signed-off-by: David Jander <david@protonic.nl>
Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/bridge/tc358767.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 91f7cb56a654..46916ae30f8f 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -501,7 +501,7 @@ static int tc_pllupdate(struct tc_data *tc, unsigned int pllctrl)
 		return ret;
 
 	/* Wait for PLL to lock: up to 2.09 ms, depending on refclk */
-	usleep_range(3000, 6000);
+	usleep_range(15000, 20000);
 
 	return 0;
 }
-- 
2.39.2

[PATCH 2/2] drm: bridge: tc358767: give VSDELAY some positive value

[Lucas Stach]

From: David Jander <david@protonic.nl>

The documentation is not clear about how this delay works.
Empirical tests have shown that with a VSDELAY of 0, the first
scanline is not properly formatted in the output stream when
DSI->DP mode is used. The calculation spreadsheets from Toshiba
seem to always make this value equal to the HFP + 10 for DSI->DP
use-case. For DSI->DPI this value should be > 2 and for DPI->DP
it seems to always be 0x64.

Signed-off-by: David Jander <david@protonic.nl>
Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/bridge/tc358767.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 46916ae30f8f..9f2c67b4a488 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -817,7 +817,7 @@ static int tc_set_common_video_mode(struct tc_data *tc,
 	 * sync signals
 	 */
 	ret = regmap_write(tc->regmap, VPCTRL0,
-			   FIELD_PREP(VSDELAY, 0) |
+			   FIELD_PREP(VSDELAY, right_margin + 10) |
 			   OPXLFMT_RGB888 | FRMSYNC_DISABLED | MSF_DISABLED);
 	if (ret)
 		return ret;
-- 
2.39.2

Re: [PATCH 2/2] drm: bridge: tc358767: give VSDELAY some positive value

[Marek Vasut]

On 6/2/23 21:15, Lucas Stach wrote:
> From: David Jander <david@protonic.nl>
> 
> The documentation is not clear about how this delay works.
> Empirical tests have shown that with a VSDELAY of 0, the first
> scanline is not properly formatted in the output stream when
> DSI->DP mode is used. The calculation spreadsheets from Toshiba
> seem to always make this value equal to the HFP + 10 for DSI->DP
> use-case. For DSI->DPI this value should be > 2 and for DPI->DP
> it seems to always be 0x64.
> 
> Signed-off-by: David Jander <david@protonic.nl>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>   drivers/gpu/drm/bridge/tc358767.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 46916ae30f8f..9f2c67b4a488 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -817,7 +817,7 @@ static int tc_set_common_video_mode(struct tc_data *tc,
>   	 * sync signals
>   	 */
>   	ret = regmap_write(tc->regmap, VPCTRL0,
> -			   FIELD_PREP(VSDELAY, 0) |
> +			   FIELD_PREP(VSDELAY, right_margin + 10) |
>   			   OPXLFMT_RGB888 | FRMSYNC_DISABLED | MSF_DISABLED);
>   	if (ret)
>   		return ret;

Aren't you running into a problem due to VS timing misconfiguration on 
the scanout engine or DSI serializer side ? The VSDELAY seems to 
increase the length of VSYNC active . Which DSI bus mode do you use, 
sync events/pulses/burst ?

Re: [PATCH 2/2] drm: bridge: tc358767: give VSDELAY some positive value

[Lucas Stach]

Am Freitag, dem 02.06.2023 um 23:34 +0200 schrieb Marek Vasut:
> On 6/2/23 21:15, Lucas Stach wrote:
> > From: David Jander <david@protonic.nl>
> > 
> > The documentation is not clear about how this delay works.
> > Empirical tests have shown that with a VSDELAY of 0, the first
> > scanline is not properly formatted in the output stream when
> > DSI->DP mode is used. The calculation spreadsheets from Toshiba
> > seem to always make this value equal to the HFP + 10 for DSI->DP
> > use-case. For DSI->DPI this value should be > 2 and for DPI->DP
> > it seems to always be 0x64.
> > 
> > Signed-off-by: David Jander <david@protonic.nl>
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> >   drivers/gpu/drm/bridge/tc358767.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> > index 46916ae30f8f..9f2c67b4a488 100644
> > --- a/drivers/gpu/drm/bridge/tc358767.c
> > +++ b/drivers/gpu/drm/bridge/tc358767.c
> > @@ -817,7 +817,7 @@ static int tc_set_common_video_mode(struct tc_data *tc,
> >   	 * sync signals
> >   	 */
> >   	ret = regmap_write(tc->regmap, VPCTRL0,
> > -			   FIELD_PREP(VSDELAY, 0) |
> > +			   FIELD_PREP(VSDELAY, right_margin + 10) |
> >   			   OPXLFMT_RGB888 | FRMSYNC_DISABLED | MSF_DISABLED);
> >   	if (ret)
> >   		return ret;
> 
> Aren't you running into a problem due to VS timing misconfiguration on 
> the scanout engine or DSI serializer side ? The VSDELAY seems to 
> increase the length of VSYNC active .
> 

No, as far as I understand the VSDELAY adds an offset between input an
output side of the video FIFO. It shouldn't increase the length of any
sync signal, but delays the read side of the FIFO, so the write (DSI)
side has some margin to put data into the FIFO before DP side starts to
assemble packets.

>  Which DSI bus mode do you use, sync events/pulses/burst ?

At the time when this patch was written it still was the SYNC_PULSE
mode.

Regards,
Lucas

Re: [PATCH 2/2] drm: bridge: tc358767: give VSDELAY some positive value

[Marek Vasut]

On 6/7/23 14:53, Lucas Stach wrote:
> Am Freitag, dem 02.06.2023 um 23:34 +0200 schrieb Marek Vasut:
>> On 6/2/23 21:15, Lucas Stach wrote:
>>> From: David Jander <david@protonic.nl>
>>>
>>> The documentation is not clear about how this delay works.
>>> Empirical tests have shown that with a VSDELAY of 0, the first
>>> scanline is not properly formatted in the output stream when
>>> DSI->DP mode is used. The calculation spreadsheets from Toshiba
>>> seem to always make this value equal to the HFP + 10 for DSI->DP
>>> use-case. For DSI->DPI this value should be > 2 and for DPI->DP
>>> it seems to always be 0x64.
>>>
>>> Signed-off-by: David Jander <david@protonic.nl>
>>> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
>>> ---
>>>    drivers/gpu/drm/bridge/tc358767.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
>>> index 46916ae30f8f..9f2c67b4a488 100644
>>> --- a/drivers/gpu/drm/bridge/tc358767.c
>>> +++ b/drivers/gpu/drm/bridge/tc358767.c
>>> @@ -817,7 +817,7 @@ static int tc_set_common_video_mode(struct tc_data *tc,
>>>    	 * sync signals
>>>    	 */
>>>    	ret = regmap_write(tc->regmap, VPCTRL0,
>>> -			   FIELD_PREP(VSDELAY, 0) |
>>> +			   FIELD_PREP(VSDELAY, right_margin + 10) |
>>>    			   OPXLFMT_RGB888 | FRMSYNC_DISABLED | MSF_DISABLED);
>>>    	if (ret)
>>>    		return ret;
>>
>> Aren't you running into a problem due to VS timing misconfiguration on
>> the scanout engine or DSI serializer side ? The VSDELAY seems to
>> increase the length of VSYNC active .
>>
> 
> No, as far as I understand the VSDELAY adds an offset between input an
> output side of the video FIFO. It shouldn't increase the length of any
> sync signal, but delays the read side of the FIFO, so the write (DSI)
> side has some margin to put data into the FIFO before DP side starts to
> assemble packets.

Does this apply to DSI-to-DPI mode too ?

>>   Which DSI bus mode do you use, sync events/pulses/burst ?
> 
> At the time when this patch was written it still was the SYNC_PULSE
> mode.

Can you please double-check this with current burst mode ?

Re: [PATCH 2/2] drm: bridge: tc358767: give VSDELAY some positive value

[Lucas Stach]

Am Mittwoch, dem 07.06.2023 um 15:54 +0200 schrieb Marek Vasut:
> On 6/7/23 14:53, Lucas Stach wrote:
> > Am Freitag, dem 02.06.2023 um 23:34 +0200 schrieb Marek Vasut:
> > > On 6/2/23 21:15, Lucas Stach wrote:
> > > > From: David Jander <david@protonic.nl>
> > > > 
> > > > The documentation is not clear about how this delay works.
> > > > Empirical tests have shown that with a VSDELAY of 0, the first
> > > > scanline is not properly formatted in the output stream when
> > > > DSI->DP mode is used. The calculation spreadsheets from Toshiba
> > > > seem to always make this value equal to the HFP + 10 for DSI->DP
> > > > use-case. For DSI->DPI this value should be > 2 and for DPI->DP
> > > > it seems to always be 0x64.
> > > > 
> > > > Signed-off-by: David Jander <david@protonic.nl>
> > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > > > ---
> > > >    drivers/gpu/drm/bridge/tc358767.c | 2 +-
> > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> > > > index 46916ae30f8f..9f2c67b4a488 100644
> > > > --- a/drivers/gpu/drm/bridge/tc358767.c
> > > > +++ b/drivers/gpu/drm/bridge/tc358767.c
> > > > @@ -817,7 +817,7 @@ static int tc_set_common_video_mode(struct tc_data *tc,
> > > >    	 * sync signals
> > > >    	 */
> > > >    	ret = regmap_write(tc->regmap, VPCTRL0,
> > > > -			   FIELD_PREP(VSDELAY, 0) |
> > > > +			   FIELD_PREP(VSDELAY, right_margin + 10) |
> > > >    			   OPXLFMT_RGB888 | FRMSYNC_DISABLED | MSF_DISABLED);
> > > >    	if (ret)
> > > >    		return ret;
> > > 
> > > Aren't you running into a problem due to VS timing misconfiguration on
> > > the scanout engine or DSI serializer side ? The VSDELAY seems to
> > > increase the length of VSYNC active .
> > > 
> > 
> > No, as far as I understand the VSDELAY adds an offset between input an
> > output side of the video FIFO. It shouldn't increase the length of any
> > sync signal, but delays the read side of the FIFO, so the write (DSI)
> > side has some margin to put data into the FIFO before DP side starts to
> > assemble packets.
> 
> Does this apply to DSI-to-DPI mode too ?

I guess it does the same thing, but the technical documents I have
don't really say anything about it. I don't think the VSDELAY really
matters in the DPI output case, as DPI is much slower in pulling data
out of the FIFO compared to the DP packetizing, so the DSI side should
always be able to keep up with the DPI side.

> 
> > >   Which DSI bus mode do you use, sync events/pulses/burst ?
> > 
> > At the time when this patch was written it still was the SYNC_PULSE
> > mode.
> 
> Can you please double-check this with current burst mode ?

It works fine on a hardware in DSI to DPI mode. I could check that
things are still as expected in DSI to DP mode later, but I don't
expect any issues.

Regards,
Lucas

Re: [PATCH 2/2] drm: bridge: tc358767: give VSDELAY some positive value

[Marek Vasut]

On 6/8/23 10:11, Lucas Stach wrote:
> Am Mittwoch, dem 07.06.2023 um 15:54 +0200 schrieb Marek Vasut:
>> On 6/7/23 14:53, Lucas Stach wrote:
>>> Am Freitag, dem 02.06.2023 um 23:34 +0200 schrieb Marek Vasut:
>>>> On 6/2/23 21:15, Lucas Stach wrote:
>>>>> From: David Jander <david@protonic.nl>
>>>>>
>>>>> The documentation is not clear about how this delay works.
>>>>> Empirical tests have shown that with a VSDELAY of 0, the first
>>>>> scanline is not properly formatted in the output stream when
>>>>> DSI->DP mode is used. The calculation spreadsheets from Toshiba
>>>>> seem to always make this value equal to the HFP + 10 for DSI->DP
>>>>> use-case. For DSI->DPI this value should be > 2 and for DPI->DP
>>>>> it seems to always be 0x64.
>>>>>
>>>>> Signed-off-by: David Jander <david@protonic.nl>
>>>>> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
>>>>> ---
>>>>>     drivers/gpu/drm/bridge/tc358767.c | 2 +-
>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
>>>>> index 46916ae30f8f..9f2c67b4a488 100644
>>>>> --- a/drivers/gpu/drm/bridge/tc358767.c
>>>>> +++ b/drivers/gpu/drm/bridge/tc358767.c
>>>>> @@ -817,7 +817,7 @@ static int tc_set_common_video_mode(struct tc_data *tc,
>>>>>     	 * sync signals
>>>>>     	 */
>>>>>     	ret = regmap_write(tc->regmap, VPCTRL0,
>>>>> -			   FIELD_PREP(VSDELAY, 0) |
>>>>> +			   FIELD_PREP(VSDELAY, right_margin + 10) |
>>>>>     			   OPXLFMT_RGB888 | FRMSYNC_DISABLED | MSF_DISABLED);
>>>>>     	if (ret)
>>>>>     		return ret;
>>>>
>>>> Aren't you running into a problem due to VS timing misconfiguration on
>>>> the scanout engine or DSI serializer side ? The VSDELAY seems to
>>>> increase the length of VSYNC active .
>>>>
>>>
>>> No, as far as I understand the VSDELAY adds an offset between input an
>>> output side of the video FIFO. It shouldn't increase the length of any
>>> sync signal, but delays the read side of the FIFO, so the write (DSI)
>>> side has some margin to put data into the FIFO before DP side starts to
>>> assemble packets.
>>
>> Does this apply to DSI-to-DPI mode too ?
> 
> I guess it does the same thing, but the technical documents I have
> don't really say anything about it. I don't think the VSDELAY really
> matters in the DPI output case, as DPI is much slower in pulling data
> out of the FIFO compared to the DP packetizing, so the DSI side should
> always be able to keep up with the DPI side.
> 
>>
>>>>    Which DSI bus mode do you use, sync events/pulses/burst ?
>>>
>>> At the time when this patch was written it still was the SYNC_PULSE
>>> mode.
>>
>> Can you please double-check this with current burst mode ?
> 
> It works fine on a hardware in DSI to DPI mode. I could check that
> things are still as expected in DSI to DP mode later, but I don't
> expect any issues.

Tested-by: Marek Vasut <marex@denx.de> # TC9595
Reviewed-by: Marek Vasut <marex@denx.de>

Sorry for the delay

Re: [PATCH 1/2] drm: bridge: tc358767: increase PLL lock time delay

[Marek Vasut]

On 6/2/23 21:15, Lucas Stach wrote:
> From: David Jander <david@protonic.nl>
> 
> The PLL often fails to lock with this delay. The new value was
> determined by trial and error increasing the delay bit by bit
> until the error did not occurr anymore even after several tries.
> Then double that value was taken as the minimum delay to be safe.
> 
> Signed-off-by: David Jander <david@protonic.nl>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>   drivers/gpu/drm/bridge/tc358767.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 91f7cb56a654..46916ae30f8f 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -501,7 +501,7 @@ static int tc_pllupdate(struct tc_data *tc, unsigned int pllctrl)
>   		return ret;
>   
>   	/* Wait for PLL to lock: up to 2.09 ms, depending on refclk */
> -	usleep_range(3000, 6000);
> +	usleep_range(15000, 20000);

The comment above does not seem to match either value, please fix.

With that fixed:

Reviewed-by: Marek Vasut <marex@denx.de>

Re: [PATCH 1/2] drm: bridge: tc358767: increase PLL lock time delay

[Marek Vasut]

On 6/2/23 21:15, Lucas Stach wrote:
> From: David Jander <david@protonic.nl>
> 
> The PLL often fails to lock with this delay. The new value was
> determined by trial and error increasing the delay bit by bit
> until the error did not occurr anymore even after several tries.
> Then double that value was taken as the minimum delay to be safe.
> 
> Signed-off-by: David Jander <david@protonic.nl>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

Tested-by: Marek Vasut <marex@denx.de> # TC9595
Reviewed-by: Marek Vasut <marex@denx.de>