From: Aradhya Bhatia <a-bhatia1@ti.com>

Once the DSI Link and DSI Phy are initialized, the code needs to wait
for Clk and Data Lanes to be ready, before continuing configuration.
This is in accordance with the DSI Start-up procedure, found in the
Technical Reference Manual of Texas Instrument's J721E SoC[0] which
houses this DSI TX controller.

If the previous bridge (or crtc/encoder) are configured pre-maturely,
the input signal FIFO gets corrupt. This introduces a color-shift on the
display.

Allow the driver to wait for the clk and data lanes to get ready during
DSI enable.

[0]: See section 12.6.5.7.3 "Start-up Procedure" in J721E SoC TRM
     TRM Link: http://www.ti.com/lit/pdf/spruil1

Fixes: e19233955d9e ("drm/bridge: Add Cadence DSI driver")
Tested-by: Dominik Haller <d.haller@phytec.de>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index e4c0968313af..284c468db6c3 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -767,7 +767,7 @@ static void cdns_dsi_bridge_enable(struct drm_bridge *bridge)
 	struct phy_configure_opts_mipi_dphy *phy_cfg = &output->phy_opts.mipi_dphy;
 	unsigned long tx_byte_period;
 	struct cdns_dsi_cfg dsi_cfg;
-	u32 tmp, reg_wakeup, div;
+	u32 tmp, reg_wakeup, div, status;
 	int nlanes;
 
 	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
@@ -784,6 +784,19 @@ static void cdns_dsi_bridge_enable(struct drm_bridge *bridge)
 	cdns_dsi_init_link(dsi);
 	cdns_dsi_hs_init(dsi);
 
+	/*
+	 * Now that the DSI Link and DSI Phy are initialized,
+	 * wait for the CLK and Data Lanes to be ready.
+	 */
+	tmp = CLK_LANE_RDY;
+	for (int i = 0; i < nlanes; i++)
+		tmp |= DATA_LANE_RDY(i);
+
+	if (readl_poll_timeout(dsi->regs + MCTL_MAIN_STS, status,
+			       status & tmp, 100, 500000))
+		dev_err(dsi->base.dev,
+			"Timed Out: DSI-DPhy Clock and Data Lanes not ready.\n");
+
 	writel(HBP_LEN(dsi_cfg.hbp) | HSA_LEN(dsi_cfg.hsa),
 	       dsi->regs + VID_HSIZE1);
 	writel(HFP_LEN(dsi_cfg.hfp) | HACT_LEN(dsi_cfg.hact),
-- 
2.34.1

Hi Aradhya,

Thanks for the patch.

On 20/10/24 01:24, Aradhya Bhatia wrote:
> From: Aradhya Bhatia <a-bhatia1@ti.com>

[...]

> +	/*
> +	 * Now that the DSI Link and DSI Phy are initialized,
> +	 * wait for the CLK and Data Lanes to be ready.
> +	 */
> +	tmp = CLK_LANE_RDY;
> +	for (int i = 0; i < nlanes; i++)
> +		tmp |= DATA_LANE_RDY(i);
> +
> +	if (readl_poll_timeout(dsi->regs + MCTL_MAIN_STS, status,
> +			       status & tmp, 100, 500000))

The above would mark the condition as true even if one data lane gets ready. I
think we need to poll until all data lanes are marked as ready. Also good to
give a warning in case we time out.

IMHO below should fix this:
        WARN_ON_ONCE(readl_poll_timeout(dsi->regs + MCTL_MAIN_STS, status,
                                       (tmp == (status & tmp)), 100, 0));

Regards
Devarsh

On 22/10/24 11:55, Devarsh Thakkar wrote:
> Hi Aradhya,
> 
> Thanks for the patch.
> 
> On 20/10/24 01:24, Aradhya Bhatia wrote:
>> From: Aradhya Bhatia <a-bhatia1@ti.com>
> 
> [...]
> 
>> +	/*
>> +	 * Now that the DSI Link and DSI Phy are initialized,
>> +	 * wait for the CLK and Data Lanes to be ready.
>> +	 */
>> +	tmp = CLK_LANE_RDY;
>> +	for (int i = 0; i < nlanes; i++)
>> +		tmp |= DATA_LANE_RDY(i);
>> +
>> +	if (readl_poll_timeout(dsi->regs + MCTL_MAIN_STS, status,
>> +			       status & tmp, 100, 500000))
> 
> The above would mark the condition as true even if one data lane gets ready. I
> think we need to poll until all data lanes are marked as ready. Also good to
> give a warning in case we time out.
> 
> IMHO below should fix this:
>         WARN_ON_ONCE(readl_poll_timeout(dsi->regs + MCTL_MAIN_STS, status,
>                                        (tmp == (status & tmp)), 100, 0));
> 

That's how the condition should be, yes! Thanks for the catch!

I would still prefer to keep dev_err instead of WARN_ON_ONCE, because
the latter stack-dumps during boot once, and then can never be seen
again during multiple modesets. The noise in the dmesg is not worth
the issue either.

With dev_err, it can show a clear print once every time it times out.

-- 
Regards
Aradhya