Re: [V3, 2/2] drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 and SN65DSI84 driver

[Mike Looijmans <mike.looijmans@topic.nl>]

Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topicproducts.com
W: www.topic.nl

Please consider the environment before printing this e-mail
On 25-05-2021 16:42, Marek Vasut wrote:
> On 5/25/21 4:23 PM, Mike Looijmans wrote:
>
> [...]
>
>>>> Alternatively, one can modify the RPi DSI code to start sending 
>>>> data after the enable calls. That also works on my setup, with 
>>>> everything in enable.
>>>>
>>>> The major point here is that you should pick one and only one 
>>>> callback: pre-enable or enable. The GPIO reset code as well as 
>>>> writing the registers should be done in that one method.
>>>
>>> Why , please elaborate ? It seems to be if there was no need for 
>>> those two callbacks, there would be no two callbacks in the API in 
>>> the first place. There is a chance you will get disable()->enable() 
>>> sequence without going through post_disable()->pre_enable() as far 
>>> as I can tell.
>>
>> The datasheet states that "the DSI CLK lanes MUST be in HS state and 
>> the DSI data lanes MUST be driven to LP11 state" when the reset 
>> de-asserts (goes high) and when the CSR registers are being written.
>>
>> Your driver now de-asserts the reset in the pre_enable and writes the 
>> CSR registers in enable. This is the "least likely to work" option.
>
> Understood. However, it seems to work on iMX8MM and MN just fine.
>
> Is there a problem on the RPi, that the driver does not work on it ?

On the RPi, without any changes, the board won't output anything. Only 
the test pattern works (using i2cset to enable it). There are two ways 
to make it work:
- Use the current RPi DSI driver as is, and move all initialization code 
in the sn65dsi83 driver to the "pre_enable" callback.

- Change the RPi DSI driver to not send any video data until after 
"enable" and move all initialization code in the sn65dsi83 driver to the 
"enable" callback.


There's also a bug in the RPi DSI driver that it does not call the 
mode_set and mode_valid methods, so it needed workarounds for that too. 
But that should be fixed in the RPi DSI driver and not in your code.

It's a surprise to me then that it works on the iMX. On the RPi it 
absolutely won't work if the DSI is sending video data when the CSR 
registers are being written. It does not seem to matter whether the data 
lanes are in LP00 or LP11 state though, either mode appears to work.

Maybe the iMX is still in a command mode and doesn't actually start 
sending video until after the "enable" callback?


Given that the test-pattern mode does work regardless of the data lane 
state, it appears that the DSI input of the sn65dsi83 chip goes berserk 
when the video data arrives before the CSR registers were written. It 
won't recover from that without a full reset.


>
>> Both the reset and the CSR writing are to be done in the same 
>> context. So either everything in "pre_enable", or everything in 
>> "enable". Which one is correct is up to the maintainers, I also don't 
>> know which is best. The other callback can just remain unused.
>>
>> If the choice is to do the chip initialization in "pre_enable" then 
>> do all the de-initialization in "post_disable". If the choice is to 
>> do the chip initialization in "enable" then do all the 
>> de-initialization in "disable".
>>
>> If for some platform the choice happens to be wrong, it's a very 
>> simple patch (just change the "ops" pointers) to change it and make 
>> it work for that platform.
>>
>> Alternatively, it's possible to make it a runtime choice through 
>> devicetree or so as to whether the CSR initializes at "enable" or 
>> "pre-enable".
>
> That would mean you encode policy in DT, so not an option.
>
> I would suggest we stop this discussion until there is input from the 
> maintainers. It could even be there is an API missing for configuring 
> the clock/data/LP/HS modes which needs to be added.

Agree...

There's no easy way out here. Adding a "post_pre_enable" or 
"pre_enable_clock_up_data_down" callback is not going to make it I guess...

> [...]


-- 
Mike Looijmans