Re: [PATCH v4 07/11] drm/bridge: cdns-dsi: Reset the DCS write FIFO

[Aradhya Bhatia <a-bhatia1@ti.com>]

On 26/06/24 16:33, Tomi Valkeinen wrote:
> On 22/06/2024 14:09, Aradhya Bhatia wrote:
>> If any normal DCS write command has already been transmitted prior to
>> transmitting any Zero-Parameter DCS command, then it is necessary to
>> clear the TX FIFO by resetting it. Otherwise, the FIFO points to another
>> location, and the DCS command transmits unnecessary data causing the
>> panel to not work[0].
>>
>> Allow the DCS Write FIFO in the cdns-dsi controller to reset as a rule,
>> before any DCS packet is transmitted to the DSI peripheral.
>>
>> [0]: Section 12.6.5.7.5.2: "Command Mode Settings" in TDA4VM Technical
>>       Reference Manual: https://www.ti.com/lit/zip/spruil1
> 
> Hmm so if I read the doc right, it says: if sending zero-parameter dcs
> command, clear the FIFO and write zero to direct_cmd_wrdat.

That's right.

> 
> Your patch seems to always clear the FIFO, not only for zero-parameter
> commands. Is that a problem (I don't think so, but...)?
> 

My patch does clear the FIFO every time.

While there is no documentation that says its harmless, I have tested
the patches with RPi Panel (which doesn't seem to have any
zero-parameter commands in the driver) - and so far it seems to have
worked fine.


> Also, is the direct_cmd_wrdat written at all when sending zero-parameter
> dcs command?
> 

At the moment, no.

Apparently there are 2 types of "Zero parameter" commands.

There is,

a) "MIPI_DSI_GENERIC_SHORT_WRITE_0_PARAM" - which has absolutely no
parameter that needs to be sent, and there is,

b) "MIPI_DSI_DCS_SHORT_WRITE" - which has a 1-byte command value that
needs to be transmitted.

(Macros referred from mipi_display.h)

In the J721E TRM[0], there is a table[1]  which classifies the
"MIPI_DSI_DCS_SHORT_WRITE" - the command with 1-byte command parameter -
as a "zero parameter" command.

For a "MIPI_DSI_DCS_SHORT_WRITE" command, we are still writing the
1-byte command data into the FIFO.

However, in the other section which talks about resetting the FIFO[2],
it is mentioned that, for "zero parameter" commands, the FIFO needs to
be reset and then 0x00 needs to be written to the FIFO.

The second step cannot be done for "MIPI_DSI_DCS_SHORT_WRITE" commands
because we want to write the 1 byte command parameter instead of 0x00
into the FIFO.

So, the only logical conclusion is that, the FIFO reset is only required
for _truly_ zero parameter commands, which is the
"MIPI_DSI_GENERIC_SHORT_WRITE_0_PARAM" command.

So, I am planning to change this patch to do 2 things, under the
condition that there are absolutely no data bytes that require
transmission.

a. Reset the FIFO.
b. Write 0x00 to the FIFO.


Regards
Aradhya

[0]: J721E TRM: https://www.ti.com/lit/zip/spruil1
[1]: Table: 12-1933: "DSI Main Settings Register Description".
[2]: Section 12.6.5.7.5.2: "Command Mode Settings"


> 
>>
>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>> ---
>>   drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> index 126e4bccd868..cad0c1478ef0 100644
>> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> @@ -1018,6 +1018,9 @@ static ssize_t cdns_dsi_transfer(struct
>> mipi_dsi_host *host,
>>         cdns_dsi_init_link(dsi);
>>   +    /* Reset the DCS Write FIFO */
>> +    writel(0x00, dsi->regs + DIRECT_CMD_FIFO_RST);
>> >>       ret = mipi_dsi_create_packet(&packet, msg);
>>       if (ret)
>>           goto out;
> 
>