[PATCH 2/6] drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct adjusted_mode clock

[Marek Vasut]

On 6/5/24 12:52 PM, Alexander Stein wrote:
> Hi Marek,

Hi,

>>>>> The accuracy degrades with each call, until a full kHz frequency is reached,
>>>>> because drm_display_mode.clock only accounts for kHz, but the PLL
>>>>> calculation takes Hz into account.
>>>>
>>>> Hmmmmm, I need to take a closer look at this one.
>>>>
>>>> Do you have any quick hints ?
>>
>> So the fix is real simple, try this extra diff:
>>
>> diff --git a/drivers/gpu/drm/bridge/tc358767.c
>> b/drivers/gpu/drm/bridge/tc358767.c
>> index 32639865fea07..5d76285dcf245 100644
>> --- a/drivers/gpu/drm/bridge/tc358767.c
>> +++ b/drivers/gpu/drm/bridge/tc358767.c
>> @@ -1629,7 +1629,7 @@ static int tc_dpi_atomic_check(struct drm_bridge
>> *bridge,
>>           int ret;
>>
>>           ret = tc_pxl_pll_calc(tc, clk_get_rate(tc->refclk),
>> -                             crtc_state->adjusted_mode.clock * 1000,
>> +                             crtc_state->mode.clock * 1000,
>>                                 &adjusted_clock, NULL);
>>           if (ret)
>>                   return ret;
>> @@ -1653,7 +1653,7 @@ static int tc_edp_atomic_check(struct drm_bridge
>> *bridge,
>>           int ret;
>>
>>           ret = tc_pxl_pll_calc(tc, clk_get_rate(tc->refclk),
>> -                             crtc_state->adjusted_mode.clock * 1000,
>> +                             crtc_state->mode.clock * 1000,
>>                                 &adjusted_clock, NULL);
>>           if (ret)
>>                   return ret;
>>
> 
> Ah, right. That seems simple. But I noticed another thing:
> The TC9595 PLL is configured to 147333333 while the lcdif configures
> the display clock to 147333000, the value actually stored in
> adjusted_mode.clock. I do not know if this small difference can be neglected.

Good point, please see (*) below.

>>> No, sorry. I'm not sure about those VFIFO overruns/underruns you mentioned
>>> in the commit message. Does this maybe only apply to DPI input?
>>
>> No, this actually happens with DSI input in both DPI and (e)DP output
>> modes, it is only really well visible in some resolutions it seems.
> 
> Ah, i see.
> 
>>> At least for 148.5MHz (1080p) apparently it is not possible to that
>>> exact clock anyway.
>>
>> Right, which sucks, but the TC9595 datasheet explicitly states that the
>> FRMSYNC mode should always be enabled (and is apparently force-enabled
>> on newer bridge chips with no option to disable it) unless the Pixel
>> clock are sourced from DSI clock (which is never the case with this
>> driver). That's what the [1] patch does.
>>
>> But messing with the HFP isn't really working for all resolutions, so
>> this patch instead adjusts the input pixel clock to fit the Pixel PLL
>> limits, which avoids the VFIFO issues altogether. And that makes the
>> FRMSYNC mode work for all kinds of resolutions.
> 
> I can't tell if VFIFO issue arise here, because I'm currently trying to
> get a reliable and stable output for DP.

What kind of problem do you observe on your side exactly ? (I think the 
answer to this is at the end). I've spent quite a while on this, so 
maybe I ran into that before.

> The documentation is somewhat
> limited, but FRAMSYNC seems almost necessary in any case, unless you
> utilize DSI bus 100% for this device to get the correct display timings
> using DSI packets.

That's not quite what it says.

I don't know what kind of datasheet you have and for which exact bridge, 
but look at Section 4, block diagram.

The FRMSYNC operates on the LS_Clk domain side, right of V_FiFo . It 
reads lines from the VFIFO and sends them out of the DP as DP packets. 
As long as the input into the VFIFO delivers the lines such that the 
VFIFO does not overflow or underflow, everything is fine.

The DSI side starts to write line into the VFIFO on HSS (Hsync start), 
see "DSI Packets for Video Transmission". HSE (Hsync end) packet is 
ignored, see "Video Transmission" . Two consecutive HSS must be sent 
with the same time between the two as time between two lines configured 
into FRMSYNC timing generator.

However, I _think_ (*) if the time between two HSS is slightly longer 
than what the FRMSYNC expects, the FRMSYNC stretches the HFP slightly 
for each frame and we won't run into any problems (see 4.12 Clocks and 
search for keyword "approximated", they talk about close approximation 
there).

>> You might be wondering why not source pixel clock from DSI HS clock and
>> disable FRMSYNC. For one thing, this would limit the ability to pick
>> faster DSI HS clock and make good use of the DSI burst mode (the DSI
>> link can go into LP state after transmitting entire line of pixel data
>> for longer with faster DSI HS clock). The other thing is, to drive the
>> Pixel PLL (not to be confused with pixel clock) from DSI HS clock, the
>> DSI HS clock have to be set to specific clock rates (13*4*7=364 MHz and
>> 13*4*9=468 MHz), which is really limiting.

btw. those 13 MHz match one of RefClk input options, the *4 is from 
HSBY4 divider and 7 and 9 come from MODE[1] strap option dividers.

> This is not mentioned in the datasheet at all, but just in a small note
> in the calculation sheet, without any description. On a different platform
> DSI HS clock running at 445.5MHz seems also possible, even if unsupported.

You can use arbitrary DSI HS clock as long as you don't derive RefClk 
from DSI HS clock (and RefClk feeds Pixel PLL).

>>>>> BTW: Which platform are you testing on?
>>>>
>>>> MX8MP with LCDIFv3 -> DSIM -> TC9595 -> DP output.
>>>>
>>>> The TC9595 is 2nd generation (automotive?) replacement for TC358767 (1st
>>>> generation replacement is TC358867) .
>>>
>>> Same here. But fail to get output on a DP monitor if I'm running from
>>> external refclk. Using DSICLK/4 seems necessary for some reason, but it
>>> is very unreliable to get a proper image.
>>
>> Do you have all the patches in place ? This is what you should have:
>>
>> drm/bridge: tc358767: Enable FRMSYNC timing generator
>> drm: bridge: samsung-dsim: Initialize bridge on attach
>> drm/bridge: tc358767: Reset chip again on attach
>> clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate
>> drm: lcdif: Use adjusted_mode .clock instead of .crtc_clock
>> drm/bridge: tc358767: Fix comment in tc_edp_mode_valid
>> drm/bridge: tc358767: Check if fully initialized before signalling HPD
>> event via IRQ
>> drm/bridge: tc358767: Split tc_pxl_pll_en() into parameter calculation
>> and enablement
>> drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct adjusted_mode clock
>> drm/bridge: tc358767: Drop line_pixel_subtract
>> drm/bridge: tc358767: Disable MIPI_DSI_CLOCK_NON_CONTINUOUS
>> drm/bridge: tc358767: Set LSCLK divider for SYSCLK to 1
>> Revert "drm/bridge: tc358767: Set default CLRSIPO count"
>> drm/bridge: tc358767: Add configurable default preemphasis
> 
> Thanks, I was missing the lcdif and clk-imx8mp patches. I saw them separately
> on the mailing list, but didn't realize would need them.
> 
>> I only use external refclk, at 26 MHz.
> 
> Same for me, difference is I'm using a crystal oscillator instead of
> CCM_CLKOUT. And yes, this is the TQ platform TQMa8MPxL/MBa8MPxL.

I also have a SoM modified to use Xtal, but it seems unnecessary, the 
CCM output saves on BoM too.

[...]

> Thanks for sharing, despite the fixed-clock and IMX8MP_CLK_CLKOUT2 this
> looks very much the same. Unfortunately I get an output on DP just sometimes.
> As Bit 0 in register 0x464 is not cleared, I suspect the bridge is not
> recognizing DSI (VSYNC) packets properly :(
> At some point this bridge is lenient, but picky otherwise.

You did strap the bridge such that it sources RefClk from the Xtal, 
right (MODE[0] = 0 -> RefClk)? I haven't seen VFUEN[0] lock up like that 
before.

You could try and force the bridge to generate test pattern instead of 
DSI_RX, edit the tc358767.c -> locate tc_dsi_rx_enable() -> locate 
tc_test_pattern -> make sure the branch option value |= 
DP0_VIDSRC_COLOR_BAR is activated . Rebuild and retest .

This will allow you to validate the TC358767<->DP connection separately 
from the DSI<->TC358767 connection . This is what I did when I was 
debugging DSI issues on another board with TC9595 . Once you are sure 
one side of the connection is stable, you can focus on the other side.

Also, with the COLOR_BAR mode in place, try experimenting with the DP 
preemphasis tuning and possibly the initial differential voltage tuning 
(it is in the same registers as the preemphasis). Maybe you have link 
quality issues and the link is borderline, it does train and link up, 
but fails right after. Increasing the preemphasis and differential 
voltage might help stabilize it.

Also, I use these to fiddle with the TC9595 (in my case) DP timing 
registers, the first one reads them all out, the second one writes them. 
Aadjust values as needed, the values below are from whatever experiment 
so they are likely invalid. Make specific note of the last 0x01 on the 
second line, that's the VFUEN update:
$ i2ctransfer -f -y 2 w2 at 0xf 0x4 0x50 r24
$ i2ctransfer -f -y 2 w26 at 0xf 0x4 0x50   0x10 0x01 0x50 0x01   0x20 0x00 
0x50 0x00   0x80 0x07 0x20 0x00   0x06 0x00 0x1a 0x00   0xb0 0x04 0x03 
0x00   0x01 0x00 0x00 0x00

>> // This is to let LCDIF configure the pixel clock,
>> // it removes the fixed Video PLL configuration from DT
>> &media_blk_ctrl {
>>          assigned-clocks = <&clk IMX8MP_CLK_MEDIA_AXI>,
>>                            <&clk IMX8MP_CLK_MEDIA_APB>,
>>                            <&clk IMX8MP_CLK_MEDIA_DISP1_PIX>,
>>                            <&clk IMX8MP_CLK_MEDIA_DISP2_PIX>;
>>          assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>,
>>                                   <&clk IMX8MP_SYS_PLL1_800M>,
>>                                   <&clk IMX8MP_VIDEO_PLL1_OUT>,
>>                                   <&clk IMX8MP_SYS_PLL3_OUT>;
>>          assigned-clock-rates = <500000000>, <200000000>,
>>                                 <0>, <0>;
>> };
> 
> I get the intention, but this might change once you want to enable ISP/DWE.
> But that is a different matter for now.

ACK

[...]