Re: [PATCH V2 0/3] DSI host and peripheral initialisation ordering

[Frieder Schrempf <frieder.schrempf@kontron.de>]

Hi Dave,

Am 06.07.22 um 12:27 schrieb Dave Stevenson:
> Hi Frieder.
> 
> Apologies Lucas - I missed your response.
> 
> On Wed, 6 Jul 2022 at 08:09, Frieder Schrempf
> <frieder.schrempf@kontron.de> wrote:
>>
>> Am 10.06.22 um 09:52 schrieb Lucas Stach:
>>> Hi,
>>>
>>> Am Mittwoch, dem 11.05.2022 um 16:58 +0200 schrieb Marek Szyprowski:
>>>> Hi Dave,
>>>>
>>>> On 05.04.2022 13:43, Dave Stevenson wrote:
>>>>> On Fri, 18 Mar 2022 at 12:25, Dave Stevenson
>>>>> <dave.stevenson@raspberrypi.com>  wrote:
>>>>>> On Fri, 4 Mar 2022 at 15:18, Dave Stevenson
>>>>>> <dave.stevenson@raspberrypi.com>  wrote:
>>>>>>> Hi All
>>>>>> A gentle ping on this series. Any comments on the approach?
>>>>>> Thanks.
>>>>> I realise the merge window has just closed and therefore folks have
>>>>> been busy, but no responses on this after a month?
>>>>>
>>>>> Do I give up and submit a patch to document that DSI is broken and no one cares?
>>>>
>>>> Thanks for pointing this patchset in the 'drm: bridge: Add Samsung MIPI
>>>> DSIM bridge' thread, otherwise I would miss it since I'm not involved
>>>> much in the DRM development.
>>>>
>>>> This resolves most of the issues in the Exynos DSI and its recent
>>>> conversion to the drm bridge framework. I've added the needed
>>>> prepare_upstream_first flags to the panels and everything works fine
>>>> without the bridge chain order hacks.
>>>>
>>>> Feel free to add:
>>>>
>>>> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>>>
>>>>
>>>> The only remaining thing to resolve is the moment of enabling DSI host.
>>>> The proper sequence is:
>>>>
>>>> 1. host power on, 2. device power on, 3. host init, 4. device init, 5.
>>>> video enable.
>>>>
>>>> #1 is done in dsi's pre_enable, #2 is done in panel's prepare. #3 was so
>>>> far done in the first host transfer call, which usually happens in
>>>> panel's prepare, then the #4 happens. Then video enable is done in the
>>>> enable callbacks.
>>>>
>>>> Jagan wants to move it to the dsi host pre_enable() to let it work with
>>>> DSI bridges controlled over different interfaces
>>>> (https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2F20220504114021.33265-6-jagan%40amarulasolutions.com%2F&amp;data=05%7C01%7Cfrieder.schrempf%40kontron.de%7Ca82478efb8434ac07dfb08da5f3a1b75%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C637927000416444951%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=vgnJ8L7fhloUV83p%2Be6ziUKHbL9apqcLWpDvMUcOOoY%3D&amp;reserved=0
>>>> ). This however fails on Exynos with DSI panels, because when dsi's
>>>> pre_enable is called, the dsi device is not yet powered. I've discussed
>>>> this with Andrzej Hajda and we came to the conclusion that this can be
>>>> resolved by adding the init() callback to the struct mipi_dsi_host_ops.
>>>> Then DSI client (next bridge or panel) would call it after powering self
>>>> on, but before sending any DSI commands in its pre_enable/prepare functions.
>>>>
>>>> I've prepared a prototype of such solution. This approach finally
>>>> resolved all the initialization issues! The bridge chain finally matches
>>>> the hardware, no hack are needed, and everything is controlled by the
>>>> DRM core. This prototype also includes the Jagan's patches, which add
>>>> IMX support to Samsung DSIM. If one is interested, here is my git repo
>>>> with all the PoC patches:
>>>>
>>>> https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmszyprow%2Flinux%2Ftree%2Fv5.18-next-20220511-dsi-rework&amp;data=05%7C01%7Cfrieder.schrempf%40kontron.de%7Ca82478efb8434ac07dfb08da5f3a1b75%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C637927000416444951%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=ka2wikbVhc4RxFGARjTh0ixGKiaNHloW9dVkejA5kEg%3D&amp;reserved=0
>>>
>>> While this needs rework on the bridge chip side, I fear that we need
>>> something like that to allow the bridge to control the sequencing of
>>> the DSI host init. While most bridges that aren't controlled via the
>>> DSI channel might be fine with just initializing the host right before
>>> a video signal is driven, there are some that need a different
>>> sequencing.
>>>
>>> The chip I'm currently looking at is a TC368767, where the datasheet
>>> states that the DSI lanes must be in LP-11 before the reset is
>>> released. While the datasheet doesn't specify what happens if that
>>> sequence is violated, Marek Vasut found that the chip enters a test
>>> mode if the lanes are not in LP-11 at that point and I can confirm this
>>> observation.
>>
>> The SN65DSI84 also has this requirement according to the datasheet.
> 
> SN65DSI8[3|4|5] need LP-11 before the chip comes out of reset, but
> that only happens as part of enabling the bridge chain. This patch set
> allows it to request the DSI host to be initialised first in order to
> meet that constraint, which is common with many DSI sink devices.
> 
> Lucas' comment with TC368767 is that it is doing other things for
> display negotiation over the AUX channel prior to enabling the video
> path, and that is needing the DSI interface to be enabled and in LP-11
> much earlier (and potentially clock lane in HS if not using an
> external clock).

Ok, got it know. I see this is a separate issue that is specific to
devices that need the link for video mode negotiation early. Thanks for
the explanation!

> 
>>> Now with the TC358767 being a DSI to (e)DP converter, we need to
>>> release the chip from reset pretty early to establish the DP AUX
>>> connection to communicate with the display, in order to find out which
>>> video modes we can drive. As the chip is controlled via i2c in my case,
>>> initializing the DSI host on first DSI command transaction is out and
>>> doing so before the bridge pre_enable is way too late.
>>>
>>> What I would need for this chip to work properly is an explicit call,
>>> like the mipi_dsi_host_init() added in the PoC above, to allow the
>>> bridge driver to kick the DSI host initialization before releasing the
>>> chip from reset state.
>>
>> So to sum up on the missing parts:
>>
>> 1. This series needs more reviews, but is generally agreed on.
>> 2. Jagan will integrate Marek's mipi_dsi_host_init() PoC when respinning
>> his series "drm: bridge: Add Samsung MIPI DSIM bridge".
> 
> I'm still not clear as to whether Marek's mipi_dsi_host_init is needed
> in the majority of cases.
> Exynos appeared to be trying to check for no contention as part of the
> initialisation to LP-11, and that isn't necessary. No one has come
> back on that one.
> 
> Adding a mipi_dsi_host_init would potentially solve the additional
> issue for TC358767.
> However it leaves a number of open questions. The first I can think of
> is that there are no defined mechanisms for specifying the link
> frequency prior to having a video mode set. Yes struct mipi_dsi_device
> has hs_rate, but that is defined as the maximum HS rate that the
> device supports, and therefore open to variation in the
> implementation. Exynos has various vendor specific DT parameters to
> configure link frequencies, which ought to be standardised if that is
> the preferred approach.

Having standardized DT properties to configure the early link parameters
could be a possible solution.

> 
>> 3. Bridge drivers need to be adjusted to call mipi_dsi_host_init() if
>> required.
> 
> Which hopefully is only the weirder devices such as TC358767.
> SN65DSI8[3|4|5] do not need this, but they will need
> pre_enable_upstream_first if/when the enable logic is shifted from
> atomic_enable to atomic_pre_enable.

Got it, thanks for clarifying!

Thanks
Frieder