[PATCH] drm/bridge: ti-sn65dsi83: Add and use hs_rate and lp_rate

[Marek Vasut]

On 8/1/22 18:38, Adam Ford wrote:
> On Mon, Aug 1, 2022 at 11:05 AM Marek Vasut <marex at denx.de> wrote:
>>
>> On 8/1/22 16:55, Dave Stevenson wrote:
>>> Hi Marek
>>
>> Hi,
>>
>>> On Mon, 1 Aug 2022 at 14:12, Marek Vasut <marex at denx.de> wrote:
>>>>
>>>> Fill in hs_rate and lp_rate to struct mipi_dsi_device for this bridge and
>>>> adjust DSI input frequency calculations such that they expect the DSI host
>>>> to configure HS clock according to hs_rate.
>>>
>>> I think this falls into another of the areas that is lacking in the DSI support.
>>> hs_rate is defined as the *maximum* lane frequency in high speed
>>> mode[1]. As documented there is no obligation on the DSI host to
>>> choose this specific rate, just some frequency below it and above or
>>> equal to that required by the pixel clock. At a system level, the link
>>> frequency is often prescribed for EMC purposes.
>>
>> The reduced upper limit could likely be defined by a DT property, but
>> that's not hard to add.
>>
>>> The issue is then that the SN65DSI83 is configured to use the DSI
>>> clock lane as the source for the PLL generating the LVDS clock,
>>> therefore with no route for the DSI peripheral to query the link
>>> frequency chosen by the host, you're stuck.
>>
>> At least making the host pick the highest supported frequency means we
>> have a well defined link frequency which either is accepted by both ends
>> or not at all, instead of the current guesswork on both ends, bridge and
>> host.
>>
>> Regarding reduction of the maximum hs_rate, see above, probably use a DT
>> property. Regarding check for hs_rate below minimum possible rate,
>> likely something the DSI host should do based on desired mode .
>>
>> If you are looking for some frequency negotiation starter, look at:
>> [RFC][PATCH 0/7] drm/bridge: Add support for selecting DSI host HS clock
>> from DSI bridge
>>
>>> SN65DSI83 also supports non-burst mode (as currently), so how would
>>> this be configured now?
>>> Does MIPI_DSI_MODE_VIDEO_BURST [2] oblige the DSI host to run in burst
>>> mode, or say that burst mode is supported by the peripheral?
>>
>> My understanding is that it is the former -- if the flag is set, the DSI
>> host must operate the device in burst mode.
>>
>>> What if
>>> your DSI host doesn't support burst mode and it is optional on your
>>> peripheral?
>>
>> The limit still applies. In the sync pulses mode, you can still run the
>> link at high frequency, it's just that the DSI data lanes won't go into
>> LP mode between bursts of data, they would stuff the link with NOPs
>> instead, as far as I can tell.
>>
>>> I raised these questions and others at [3], but largely got no real answers.
>>>
>>>
>>> The patch does exactly what you describe and has value, but without
>>> definition in the frameworks of exactly how burst mode must be
>>> implemented by the DSI host, I would say that it's going to cause
>>> issues.
>>> I am aware of users of your driver with the Raspberry Pi, but your
>>> expectation isn't currently valid on the Pi as there is no definition
>>> of the correct thing for the host to do.
>>
>> This patch actually helped me deal with stability issues on MX8M . Of
> 
> I have a board with the SN65DSI83 and I have been testing Jagan's DSIM
> patch series and I noticed the SN65DSI83  throws a PLL timeout error.
> What kind of stability issues were you seeing?   I'll try to pull in
> this patch, and I'm happy to test and reply with 'Tested-by' if it
> works.

You need to make sure the DSIM generates these 500 MHz HS clock (PLL at 
1 GHz, P/M/S 3/125/0 I think). I still use the older DSIM driver with 
this kind of patch:

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c 
b/drivers/gpu/drm/bridge/samsung-dsim.c
index ddadce208d38b..11f6aaf741466 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -1593,6 +1593,19 @@ static int samsung_dsim_host_attach(struct 
mipi_dsi_host *host,
         dsi->format = device->format;
         dsi->mode_flags = device->mode_flags;

+       /*
+        * In case the nearest bridge specifies DSI HS clock rate and 
supports
+        * DSI burst mode, run the DSI link at highest supported DSI HS 
clock
+        * frequency to achieve the shortest transfer bursts, longest 
time in
+        * LP mode between bursts, and thus most power efficient transfer.
+        *
+        * Note that the DSIM PLL runs at 2x HS clock rate.
+        */
+       if (device->hs_rate && (device->mode_flags & 
MIPI_DSI_MODE_VIDEO_BURST))
+               dsi->burst_clk_rate = 2 * device->hs_rate;
+       if (device->lp_rate)
+               dsi->esc_clk_rate = device->lp_rate;
+