[PATCH 1/6] drm/bridge: tc358767: Split tc_pxl_pll_en() into parameter calculation and enablement

[PATCH 1/6] drm/bridge: tc358767: Split tc_pxl_pll_en() into parameter calculation and enablement

[Marek Vasut]

Split tc_pxl_pll_en() into tc_pxl_pll_calc() which does only Pixel PLL
parameter calculation and tc_pxl_pll_en() which calls tc_pxl_pll_calc()
and then configures the Pixel PLL register.

This is a preparatory patch for further rework, where tc_pxl_pll_calc()
will also be used to find out the exact clock frequency generated by the
Pixel PLL. This frequency will be used as adjusted_mode clock frequency
and passed down the display pipeline to obtain exactly this frequency
on input into this bridge.

The precise input frequency that matches the Pixel PLL frequency is
important for this bridge, as if the frequencies do not match, the
bridge does suffer VFIFO overruns or underruns.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Andrzej Hajda <andrzej.hajda@intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: David Airlie <airlied@gmail.com>
Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Neil Armstrong <neil.armstrong@linaro.org>
Cc: Robert Foss <rfoss@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: dri-devel@lists.freedesktop.org
Cc: kernel@dh-electronics.com
---
 drivers/gpu/drm/bridge/tc358767.c | 32 ++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 029002938a5e8..45af31414ce48 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -580,9 +580,9 @@ static int tc_pllupdate(struct tc_data *tc, unsigned int pllctrl)
 	return 0;
 }
 
-static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, u32 pixelclock)
+static int tc_pxl_pll_calc(struct tc_data *tc, u32 refclk, u32 pixelclock,
+			   int *out_best_pixelclock, u32 *out_pxl_pllparam)
 {
-	int ret;
 	int i_pre, best_pre = 1;
 	int i_post, best_post = 1;
 	int div, best_div = 1;
@@ -678,11 +678,6 @@ static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, u32 pixelclock)
 	if (best_mul == 128)
 		best_mul = 0;
 
-	/* Power up PLL and switch to bypass */
-	ret = regmap_write(tc->regmap, PXL_PLLCTRL, PLLBYP | PLLEN);
-	if (ret)
-		return ret;
-
 	pxl_pllparam  = vco_hi << 24; /* For PLL VCO >= 300 MHz = 1 */
 	pxl_pllparam |= ext_div[best_pre] << 20; /* External Pre-divider */
 	pxl_pllparam |= ext_div[best_post] << 16; /* External Post-divider */
@@ -690,6 +685,29 @@ static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, u32 pixelclock)
 	pxl_pllparam |= best_div << 8; /* Divider for PLL RefClk */
 	pxl_pllparam |= best_mul; /* Multiplier for PLL */
 
+	if (out_best_pixelclock)
+		*out_best_pixelclock = best_pixelclock;
+
+	if (out_pxl_pllparam)
+		*out_pxl_pllparam = pxl_pllparam;
+
+	return 0;
+}
+
+static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, u32 pixelclock)
+{
+	u32 pxl_pllparam = 0;
+	int ret;
+
+	ret = tc_pxl_pll_calc(tc, refclk, pixelclock, NULL, &pxl_pllparam);
+	if (ret)
+		return ret;
+
+	/* Power up PLL and switch to bypass */
+	ret = regmap_write(tc->regmap, PXL_PLLCTRL, PLLBYP | PLLEN);
+	if (ret)
+		return ret;
+
 	ret = regmap_write(tc->regmap, PXL_PLLPARAM, pxl_pllparam);
 	if (ret)
 		return ret;
-- 
2.43.0

[PATCH 2/6] drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct adjusted_mode clock

[Marek Vasut]

Use tc_pxl_pll_calc() to find out the exact clock frequency generated by the
Pixel PLL. Use the Pixel PLL frequency as adjusted_mode clock frequency and
pass it down the display pipeline to obtain exactly this frequency on input
into this bridge.

The precise input frequency that matches the Pixel PLL frequency is
important for this bridge, as if the frequencies do not match, the
bridge does suffer VFIFO overruns or underruns.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Andrzej Hajda <andrzej.hajda@intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: David Airlie <airlied@gmail.com>
Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Neil Armstrong <neil.armstrong@linaro.org>
Cc: Robert Foss <rfoss@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: dri-devel@lists.freedesktop.org
Cc: kernel@dh-electronics.com
---
 drivers/gpu/drm/bridge/tc358767.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 45af31414ce48..252cc08dcc4a8 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1619,6 +1619,18 @@ static int tc_dpi_atomic_check(struct drm_bridge *bridge,
 			       struct drm_crtc_state *crtc_state,
 			       struct drm_connector_state *conn_state)
 {
+	struct tc_data *tc = bridge_to_tc(bridge);
+	int adjusted_clock = 0;
+	int ret;
+
+	ret = tc_pxl_pll_calc(tc, clk_get_rate(tc->refclk),
+			      crtc_state->adjusted_mode.clock * 1000,
+			      &adjusted_clock, NULL);
+	if (ret)
+		return ret;
+
+	crtc_state->adjusted_mode.clock = adjusted_clock / 1000;
+
 	/* DSI->DPI interface clock limitation: upto 100 MHz */
 	if (crtc_state->adjusted_mode.clock > 100000)
 		return -EINVAL;
@@ -1631,6 +1643,18 @@ static int tc_edp_atomic_check(struct drm_bridge *bridge,
 			       struct drm_crtc_state *crtc_state,
 			       struct drm_connector_state *conn_state)
 {
+	struct tc_data *tc = bridge_to_tc(bridge);
+	int adjusted_clock = 0;
+	int ret;
+
+	ret = tc_pxl_pll_calc(tc, clk_get_rate(tc->refclk),
+			      crtc_state->adjusted_mode.clock * 1000,
+			      &adjusted_clock, NULL);
+	if (ret)
+		return ret;
+
+	crtc_state->adjusted_mode.clock = adjusted_clock / 1000;
+
 	/* DPI->(e)DP interface clock limitation: upto 154 MHz */
 	if (crtc_state->adjusted_mode.clock > 154000)
 		return -EINVAL;
-- 
2.43.0

[PATCH 3/6] drm/bridge: tc358767: Drop line_pixel_subtract

[Marek Vasut]

This line_pixel_subtract is no longer needed now that the bridge can
request and obtain specific pixel clock on input to the bridge, with
clock frequency that matches the Pixel PLL frequency.

The line_pixel_subtract is now always 0, so drop it entirely.

The line_pixel_subtract was not reliable as it never worked when the
Pixel PLL and input clock were off just so that the required amount
of pixels to subtract would not be whole integer.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Andrzej Hajda <andrzej.hajda@intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: David Airlie <airlied@gmail.com>
Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Neil Armstrong <neil.armstrong@linaro.org>
Cc: Robert Foss <rfoss@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: dri-devel@lists.freedesktop.org
Cc: kernel@dh-electronics.com
---
 drivers/gpu/drm/bridge/tc358767.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 252cc08dcc4a8..f16728256991a 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -382,9 +382,6 @@ struct tc_data {
 
 	/* HPD pin number (0 or 1) or -ENODEV */
 	int			hpd_pin;
-
-	/* Number of pixels to subtract from a line due to pixel clock delta */
-	u32			line_pixel_subtract;
 };
 
 static inline struct tc_data *aux_to_tc(struct drm_dp_aux *a)
@@ -661,11 +658,7 @@ static int tc_pxl_pll_calc(struct tc_data *tc, u32 refclk, u32 pixelclock,
 		return -EINVAL;
 	}
 
-	tc->line_pixel_subtract = tc->mode.htotal -
-		DIV_ROUND_UP(tc->mode.htotal * (u64)best_pixelclock, (u64)pixelclock);
-
-	dev_dbg(tc->dev, "PLL: got %d, delta %d (subtract %d px)\n", best_pixelclock,
-		best_delta, tc->line_pixel_subtract);
+	dev_dbg(tc->dev, "PLL: got %d, delta %d\n", best_pixelclock, best_delta);
 	dev_dbg(tc->dev, "PLL: %d / %d / %d * %d / %d\n", refclk,
 		ext_div[best_pre], best_div, best_mul, ext_div[best_post]);
 
@@ -909,13 +902,6 @@ static int tc_set_common_video_mode(struct tc_data *tc,
 		upper_margin, lower_margin, vsync_len);
 	dev_dbg(tc->dev, "total: %dx%d\n", mode->htotal, mode->vtotal);
 
-	if (right_margin > tc->line_pixel_subtract) {
-		right_margin -= tc->line_pixel_subtract;
-	} else {
-		dev_err(tc->dev, "Bridge pixel clock too slow for mode\n");
-		right_margin = 0;
-	}
-
 	/*
 	 * LCD Ctl Frame Size
 	 * datasheet is not clear of vsdelay in case of DPI
-- 
2.43.0

[PATCH 4/6] drm/bridge: tc358767: Disable MIPI_DSI_CLOCK_NON_CONTINUOUS

[Marek Vasut]

The MIPI_DSI_CLOCK_NON_CONTINUOUS causes visible artifacts in high
resolution modes, disable it. Namely, in DSI->DP mode 1920x1200 24
bpp 59.95 Hz, with DSI bus at maximum 1 Gbps per lane setting, the
image contains jittering empty lines.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Andrzej Hajda <andrzej.hajda@intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: David Airlie <airlied@gmail.com>
Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Neil Armstrong <neil.armstrong@linaro.org>
Cc: Robert Foss <rfoss@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: dri-devel@lists.freedesktop.org
Cc: kernel@dh-electronics.com
---
 drivers/gpu/drm/bridge/tc358767.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index f16728256991a..926921a8d29d7 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -2303,7 +2303,7 @@ static int tc_mipi_dsi_host_attach(struct tc_data *tc)
 	dsi->lanes = dsi_lanes;
 	dsi->format = MIPI_DSI_FMT_RGB888;
 	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
-			  MIPI_DSI_MODE_LPM | MIPI_DSI_CLOCK_NON_CONTINUOUS;
+			  MIPI_DSI_MODE_LPM;
 
 	ret = devm_mipi_dsi_attach(dev, dsi);
 	if (ret < 0) {
-- 
2.43.0

[PATCH 5/6] drm/bridge: tc358767: Set LSCLK divider for SYSCLK to 1

[Marek Vasut]

The only information in the datasheet regarding this divider is a note
in SYS_PLLPARAM register documentation which states that when LSCLK is
270 MHz, LSCLK_DIV should be 1. What should LSCLK_DIV be set to when
LSCLK is 162 MHz (for DP 1.62G mode) is unclear, but empirical test
confirms using LSCLK_DIV 1 has no adverse effects either. In the worst
case, the internal TC358767 clock would run faster.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Andrzej Hajda <andrzej.hajda@intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: David Airlie <airlied@gmail.com>
Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Neil Armstrong <neil.armstrong@linaro.org>
Cc: Robert Foss <rfoss@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: dri-devel@lists.freedesktop.org
Cc: kernel@dh-electronics.com
---
 drivers/gpu/drm/bridge/tc358767.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 926921a8d29d7..156297131a6cc 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -738,7 +738,7 @@ static int tc_stream_clock_calc(struct tc_data *tc)
 static int tc_set_syspllparam(struct tc_data *tc)
 {
 	unsigned long rate;
-	u32 pllparam = SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
+	u32 pllparam = SYSCLK_SEL_LSCLK | LSCLK_DIV_1;
 
 	rate = clk_get_rate(tc->refclk);
 	switch (rate) {
-- 
2.43.0

[PATCH 6/6] Revert "drm/bridge: tc358767: Set default CLRSIPO count"

[Marek Vasut]

This reverts commit 01338bb82fed40a6a234c2b36a92367c8671adf0.

With clock improvements in place, this seems to be no longer
necessary. Set the CLRSIPO to default setting recommended by
manufacturer.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Andrzej Hajda <andrzej.hajda@intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: David Airlie <airlied@gmail.com>
Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Neil Armstrong <neil.armstrong@linaro.org>
Cc: Robert Foss <rfoss@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: dri-devel@lists.freedesktop.org
Cc: kernel@dh-electronics.com
---
 drivers/gpu/drm/bridge/tc358767.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 156297131a6cc..1243918320a7d 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1356,10 +1356,10 @@ static int tc_dsi_rx_enable(struct tc_data *tc)
 	u32 value;
 	int ret;
 
-	regmap_write(tc->regmap, PPI_D0S_CLRSIPOCOUNT, 25);
-	regmap_write(tc->regmap, PPI_D1S_CLRSIPOCOUNT, 25);
-	regmap_write(tc->regmap, PPI_D2S_CLRSIPOCOUNT, 25);
-	regmap_write(tc->regmap, PPI_D3S_CLRSIPOCOUNT, 25);
+	regmap_write(tc->regmap, PPI_D0S_CLRSIPOCOUNT, 5);
+	regmap_write(tc->regmap, PPI_D1S_CLRSIPOCOUNT, 5);
+	regmap_write(tc->regmap, PPI_D2S_CLRSIPOCOUNT, 5);
+	regmap_write(tc->regmap, PPI_D3S_CLRSIPOCOUNT, 5);
 	regmap_write(tc->regmap, PPI_D0S_ATMR, 0);
 	regmap_write(tc->regmap, PPI_D1S_ATMR, 0);
 	regmap_write(tc->regmap, PPI_TX_RX_TA, TTA_GET | TTA_SURE);
-- 
2.43.0

Re: [PATCH 3/6] drm/bridge: tc358767: Drop line_pixel_subtract

[Alexander Stein]

Hi Marek,

Am Freitag, 31. Mai 2024, 22:39:49 CEST schrieb Marek Vasut:
> This line_pixel_subtract is no longer needed now that the bridge can
> request and obtain specific pixel clock on input to the bridge, with
> clock frequency that matches the Pixel PLL frequency.
> 
> The line_pixel_subtract is now always 0, so drop it entirely.
> 
> The line_pixel_subtract was not reliable as it never worked when the
> Pixel PLL and input clock were off just so that the required amount
> of pixels to subtract would not be whole integer.

I think this is based on [1], no? I was wondering because it was not stated.

Best regards,
Alexander

[1] https://lore.kernel.org/all/20240514004759.230431-1-marex@denx.de/

> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Neil Armstrong <neil.armstrong@linaro.org>
> Cc: Robert Foss <rfoss@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: dri-devel@lists.freedesktop.org
> Cc: kernel@dh-electronics.com
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 16 +---------------
>  1 file changed, 1 insertion(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 252cc08dcc4a8..f16728256991a 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -382,9 +382,6 @@ struct tc_data {
>  
>  	/* HPD pin number (0 or 1) or -ENODEV */
>  	int			hpd_pin;
> -
> -	/* Number of pixels to subtract from a line due to pixel clock delta */
> -	u32			line_pixel_subtract;
>  };
>  
>  static inline struct tc_data *aux_to_tc(struct drm_dp_aux *a)
> @@ -661,11 +658,7 @@ static int tc_pxl_pll_calc(struct tc_data *tc, u32 refclk, u32 pixelclock,
>  		return -EINVAL;
>  	}
>  
> -	tc->line_pixel_subtract = tc->mode.htotal -
> -		DIV_ROUND_UP(tc->mode.htotal * (u64)best_pixelclock, (u64)pixelclock);
> -
> -	dev_dbg(tc->dev, "PLL: got %d, delta %d (subtract %d px)\n", best_pixelclock,
> -		best_delta, tc->line_pixel_subtract);
> +	dev_dbg(tc->dev, "PLL: got %d, delta %d\n", best_pixelclock, best_delta);
>  	dev_dbg(tc->dev, "PLL: %d / %d / %d * %d / %d\n", refclk,
>  		ext_div[best_pre], best_div, best_mul, ext_div[best_post]);
>  
> @@ -909,13 +902,6 @@ static int tc_set_common_video_mode(struct tc_data *tc,
>  		upper_margin, lower_margin, vsync_len);
>  	dev_dbg(tc->dev, "total: %dx%d\n", mode->htotal, mode->vtotal);
>  
> -	if (right_margin > tc->line_pixel_subtract) {
> -		right_margin -= tc->line_pixel_subtract;
> -	} else {
> -		dev_err(tc->dev, "Bridge pixel clock too slow for mode\n");
> -		right_margin = 0;
> -	}
> -
>  	/*
>  	 * LCD Ctl Frame Size
>  	 * datasheet is not clear of vsdelay in case of DPI
> 


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH 2/6] drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct adjusted_mode clock

[Alexander Stein]

Hi Marek,

Am Freitag, 31. Mai 2024, 22:39:48 CEST schrieb Marek Vasut:
> Use tc_pxl_pll_calc() to find out the exact clock frequency generated by the
> Pixel PLL. Use the Pixel PLL frequency as adjusted_mode clock frequency and
> pass it down the display pipeline to obtain exactly this frequency on input
> into this bridge.
> 
> The precise input frequency that matches the Pixel PLL frequency is
> important for this bridge, as if the frequencies do not match, the
> bridge does suffer VFIFO overruns or underruns.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Neil Armstrong <neil.armstrong@linaro.org>
> Cc: Robert Foss <rfoss@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: dri-devel@lists.freedesktop.org
> Cc: kernel@dh-electronics.com
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 45af31414ce48..252cc08dcc4a8 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -1619,6 +1619,18 @@ static int tc_dpi_atomic_check(struct drm_bridge *bridge,
>  			       struct drm_crtc_state *crtc_state,
>  			       struct drm_connector_state *conn_state)
>  {
> +	struct tc_data *tc = bridge_to_tc(bridge);
> +	int adjusted_clock = 0;
> +	int ret;
> +
> +	ret = tc_pxl_pll_calc(tc, clk_get_rate(tc->refclk),
> +			      crtc_state->adjusted_mode.clock * 1000,
> +			      &adjusted_clock, NULL);
> +	if (ret)
> +		return ret;
> +
> +	crtc_state->adjusted_mode.clock = adjusted_clock / 1000;
> +
>  	/* DSI->DPI interface clock limitation: upto 100 MHz */
>  	if (crtc_state->adjusted_mode.clock > 100000)
>  		return -EINVAL;
> @@ -1631,6 +1643,18 @@ static int tc_edp_atomic_check(struct drm_bridge *bridge,
>  			       struct drm_crtc_state *crtc_state,
>  			       struct drm_connector_state *conn_state)
>  {
> +	struct tc_data *tc = bridge_to_tc(bridge);
> +	int adjusted_clock = 0;
> +	int ret;
> +
> +	ret = tc_pxl_pll_calc(tc, clk_get_rate(tc->refclk),
> +			      crtc_state->adjusted_mode.clock * 1000,
> +			      &adjusted_clock, NULL);
> +	if (ret)
> +		return ret;
> +
> +	crtc_state->adjusted_mode.clock = adjusted_clock / 1000;

This is prone to rounding errors. Debug output in my case:
> [   16.007127] tc358767 1-000f: enable video stream
> [   16.007148] tc358767 1-000f: PLL: requested 148500000 pixelclock, ref 26000000
> [   16.007163] tc358767 1-000f: PLL: got 147333333, delta -1166667
> [   16.007169] tc358767 1-000f: PLL: 26000000 / 1 / 1 * 17 / 3
> [   16.027112] tc358767 1-000f: set mode 1920x1080
> [   16.027138] tc358767 1-000f: H margin 148,88 sync 44
> [   16.027144] tc358767 1-000f: V margin 36,4 sync 5
> [   16.027150] tc358767 1-000f: total: 2200x1125
> [   16.059426] tc358767 1-000f: PLL: requested 147333000 pixelclock, ref 26000000
> [   16.059455] tc358767 1-000f: PLL: got 146250000, delta -1083000
> [   16.059461] tc358767 1-000f: PLL: 26000000 / 1 / 4 * 45 / 2
> [   16.095724] tc358767 1-000f: PLL: requested 146250000 pixelclock, ref 26000000
> [   16.095739] tc358767 1-000f: PLL: got 146250000, delta 0
> [   16.095745] tc358767 1-000f: PLL: 26000000 / 1 / 4 * 45 / 2

The accuracy degrades with each call, until a full kHz frequency is reached,
because drm_display_mode.clock only accounts for kHz, but the PLL
calculation takes Hz into account.

BTW: Which platform are you testing on?

Best regards,
Alexander

> +
>  	/* DPI->(e)DP interface clock limitation: upto 154 MHz */
>  	if (crtc_state->adjusted_mode.clock > 154000)
>  		return -EINVAL;
> 


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH 3/6] drm/bridge: tc358767: Drop line_pixel_subtract

[Marek Vasut]

On 6/3/24 2:18 PM, Alexander Stein wrote:
> Hi Marek,

Hi,

> Am Freitag, 31. Mai 2024, 22:39:49 CEST schrieb Marek Vasut:
>> This line_pixel_subtract is no longer needed now that the bridge can
>> request and obtain specific pixel clock on input to the bridge, with
>> clock frequency that matches the Pixel PLL frequency.
>>
>> The line_pixel_subtract is now always 0, so drop it entirely.
>>
>> The line_pixel_subtract was not reliable as it never worked when the
>> Pixel PLL and input clock were off just so that the required amount
>> of pixels to subtract would not be whole integer.
> 
> I think this is based on [1], no?

It is.

> I was wondering because it was not stated.

I thought [1] was already applied, but it seems it was only RBd.

I can either apply [1] and then add this on top, so the two commits can 
be reverted separately if this one breaks something, or squash [1] into 
this patch and send V2. Which one do you prefer ?

The [1] fixes a real nasty issue with DPI output which causes visible 
image corruption, so I would like to have [1] in, but it is obviously 
not perfect.

Re: [PATCH 2/6] drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct adjusted_mode clock

[Marek Vasut]

On 6/3/24 2:45 PM, Alexander Stein wrote:

Hi,

>> @@ -1631,6 +1643,18 @@ static int tc_edp_atomic_check(struct drm_bridge *bridge,
>>   			       struct drm_crtc_state *crtc_state,
>>   			       struct drm_connector_state *conn_state)
>>   {
>> +	struct tc_data *tc = bridge_to_tc(bridge);
>> +	int adjusted_clock = 0;
>> +	int ret;
>> +
>> +	ret = tc_pxl_pll_calc(tc, clk_get_rate(tc->refclk),
>> +			      crtc_state->adjusted_mode.clock * 1000,
>> +			      &adjusted_clock, NULL);
>> +	if (ret)
>> +		return ret;
>> +
>> +	crtc_state->adjusted_mode.clock = adjusted_clock / 1000;
> 
> This is prone to rounding errors. Debug output in my case:
>> [   16.007127] tc358767 1-000f: enable video stream
>> [   16.007148] tc358767 1-000f: PLL: requested 148500000 pixelclock, ref 26000000
>> [   16.007163] tc358767 1-000f: PLL: got 147333333, delta -1166667
>> [   16.007169] tc358767 1-000f: PLL: 26000000 / 1 / 1 * 17 / 3
>> [   16.027112] tc358767 1-000f: set mode 1920x1080
>> [   16.027138] tc358767 1-000f: H margin 148,88 sync 44
>> [   16.027144] tc358767 1-000f: V margin 36,4 sync 5
>> [   16.027150] tc358767 1-000f: total: 2200x1125
>> [   16.059426] tc358767 1-000f: PLL: requested 147333000 pixelclock, ref 26000000
>> [   16.059455] tc358767 1-000f: PLL: got 146250000, delta -1083000
>> [   16.059461] tc358767 1-000f: PLL: 26000000 / 1 / 4 * 45 / 2
>> [   16.095724] tc358767 1-000f: PLL: requested 146250000 pixelclock, ref 26000000
>> [   16.095739] tc358767 1-000f: PLL: got 146250000, delta 0
>> [   16.095745] tc358767 1-000f: PLL: 26000000 / 1 / 4 * 45 / 2
> 
> The accuracy degrades with each call, until a full kHz frequency is reached,
> because drm_display_mode.clock only accounts for kHz, but the PLL
> calculation takes Hz into account.

Hmmmmm, I need to take a closer look at this one.

Do you have any quick hints ?

> BTW: Which platform are you testing on?

MX8MP with LCDIFv3 -> DSIM -> TC9595 -> DP output.

The TC9595 is 2nd generation (automotive?) replacement for TC358767 (1st 
generation replacement is TC358867) .

Re: [PATCH 3/6] drm/bridge: tc358767: Drop line_pixel_subtract

[Alexander Stein]

Hi Marek,

Am Montag, 3. Juni 2024, 23:25:43 CEST schrieb Marek Vasut:
> On 6/3/24 2:18 PM, Alexander Stein wrote:
> > Hi Marek,
> 
> Hi,
> 
> > Am Freitag, 31. Mai 2024, 22:39:49 CEST schrieb Marek Vasut:
> >> This line_pixel_subtract is no longer needed now that the bridge can
> >> request and obtain specific pixel clock on input to the bridge, with
> >> clock frequency that matches the Pixel PLL frequency.
> >>
> >> The line_pixel_subtract is now always 0, so drop it entirely.
> >>
> >> The line_pixel_subtract was not reliable as it never worked when the
> >> Pixel PLL and input clock were off just so that the required amount
> >> of pixels to subtract would not be whole integer.
> > 
> > I think this is based on [1], no?
> 
> It is.

Thanks for confirmation.

> > I was wondering because it was not stated.
> 
> I thought [1] was already applied, but it seems it was only RBd.
> 
> I can either apply [1] and then add this on top, so the two commits can 
> be reverted separately if this one breaks something, or squash [1] into 
> this patch and send V2. Which one do you prefer ?
> 
> The [1] fixes a real nasty issue with DPI output which causes visible 
> image corruption, so I would like to have [1] in, but it is obviously 
> not perfect.

I can't use DPI anyway, but if [1] actually fixes something for DPI
I'm okay with having [1] in, which gets mostly reverted in this series.
But that's up to the maintainers.

Best regards,
Alexander
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH 2/6] drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct adjusted_mode clock

[Alexander Stein]

Hi Marek,

Am Montag, 3. Juni 2024, 23:27:34 CEST schrieb Marek Vasut:
> On 6/3/24 2:45 PM, Alexander Stein wrote:
> 
> Hi,
> 
> >> @@ -1631,6 +1643,18 @@ static int tc_edp_atomic_check(struct drm_bridge *bridge,
> >>   			       struct drm_crtc_state *crtc_state,
> >>   			       struct drm_connector_state *conn_state)
> >>   {
> >> +	struct tc_data *tc = bridge_to_tc(bridge);
> >> +	int adjusted_clock = 0;
> >> +	int ret;
> >> +
> >> +	ret = tc_pxl_pll_calc(tc, clk_get_rate(tc->refclk),
> >> +			      crtc_state->adjusted_mode.clock * 1000,
> >> +			      &adjusted_clock, NULL);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	crtc_state->adjusted_mode.clock = adjusted_clock / 1000;
> > 
> > This is prone to rounding errors. Debug output in my case:
> >> [   16.007127] tc358767 1-000f: enable video stream
> >> [   16.007148] tc358767 1-000f: PLL: requested 148500000 pixelclock, ref 26000000
> >> [   16.007163] tc358767 1-000f: PLL: got 147333333, delta -1166667
> >> [   16.007169] tc358767 1-000f: PLL: 26000000 / 1 / 1 * 17 / 3
> >> [   16.027112] tc358767 1-000f: set mode 1920x1080
> >> [   16.027138] tc358767 1-000f: H margin 148,88 sync 44
> >> [   16.027144] tc358767 1-000f: V margin 36,4 sync 5
> >> [   16.027150] tc358767 1-000f: total: 2200x1125
> >> [   16.059426] tc358767 1-000f: PLL: requested 147333000 pixelclock, ref 26000000
> >> [   16.059455] tc358767 1-000f: PLL: got 146250000, delta -1083000
> >> [   16.059461] tc358767 1-000f: PLL: 26000000 / 1 / 4 * 45 / 2
> >> [   16.095724] tc358767 1-000f: PLL: requested 146250000 pixelclock, ref 26000000
> >> [   16.095739] tc358767 1-000f: PLL: got 146250000, delta 0
> >> [   16.095745] tc358767 1-000f: PLL: 26000000 / 1 / 4 * 45 / 2
> > 
> > The accuracy degrades with each call, until a full kHz frequency is reached,
> > because drm_display_mode.clock only accounts for kHz, but the PLL
> > calculation takes Hz into account.
> 
> Hmmmmm, I need to take a closer look at this one.
> 
> Do you have any quick hints ?

No, sorry. I'm not sure about those VFIFO overruns/underruns you mentioned
in the commit message. Does this maybe only apply to DPI input?
At least for 148.5MHz (1080p) apparently it is not possible to that
exact clock anyway.

> > BTW: Which platform are you testing on?
> 
> MX8MP with LCDIFv3 -> DSIM -> TC9595 -> DP output.
> 
> The TC9595 is 2nd generation (automotive?) replacement for TC358767 (1st 
> generation replacement is TC358867) .

Same here. But fail to get output on a DP monitor if I'm running from
external refclk. Using DSICLK/4 seems necessary for some reason, but it
is very unreliable to get a proper image.
Which display are you using? Do you mind sharing your DT?

Best regards,
Alexander
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH 2/6] drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct adjusted_mode clock

[Marek Vasut]

On 6/4/24 1:35 PM, Alexander Stein wrote:
> Hi Marek,

Hi,

> Am Montag, 3. Juni 2024, 23:27:34 CEST schrieb Marek Vasut:
>> On 6/3/24 2:45 PM, Alexander Stein wrote:
>>
>> Hi,
>>
>>>> @@ -1631,6 +1643,18 @@ static int tc_edp_atomic_check(struct drm_bridge *bridge,
>>>>    			       struct drm_crtc_state *crtc_state,
>>>>    			       struct drm_connector_state *conn_state)
>>>>    {
>>>> +	struct tc_data *tc = bridge_to_tc(bridge);
>>>> +	int adjusted_clock = 0;
>>>> +	int ret;
>>>> +
>>>> +	ret = tc_pxl_pll_calc(tc, clk_get_rate(tc->refclk),
>>>> +			      crtc_state->adjusted_mode.clock * 1000,
>>>> +			      &adjusted_clock, NULL);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	crtc_state->adjusted_mode.clock = adjusted_clock / 1000;
>>>
>>> This is prone to rounding errors. Debug output in my case:
>>>> [   16.007127] tc358767 1-000f: enable video stream
>>>> [   16.007148] tc358767 1-000f: PLL: requested 148500000 pixelclock, ref 26000000
>>>> [   16.007163] tc358767 1-000f: PLL: got 147333333, delta -1166667
>>>> [   16.007169] tc358767 1-000f: PLL: 26000000 / 1 / 1 * 17 / 3
>>>> [   16.027112] tc358767 1-000f: set mode 1920x1080
>>>> [   16.027138] tc358767 1-000f: H margin 148,88 sync 44
>>>> [   16.027144] tc358767 1-000f: V margin 36,4 sync 5
>>>> [   16.027150] tc358767 1-000f: total: 2200x1125
>>>> [   16.059426] tc358767 1-000f: PLL: requested 147333000 pixelclock, ref 26000000
>>>> [   16.059455] tc358767 1-000f: PLL: got 146250000, delta -1083000
>>>> [   16.059461] tc358767 1-000f: PLL: 26000000 / 1 / 4 * 45 / 2
>>>> [   16.095724] tc358767 1-000f: PLL: requested 146250000 pixelclock, ref 26000000
>>>> [   16.095739] tc358767 1-000f: PLL: got 146250000, delta 0
>>>> [   16.095745] tc358767 1-000f: PLL: 26000000 / 1 / 4 * 45 / 2
>>>
>>> The accuracy degrades with each call, until a full kHz frequency is reached,
>>> because drm_display_mode.clock only accounts for kHz, but the PLL
>>> calculation takes Hz into account.
>>
>> Hmmmmm, I need to take a closer look at this one.
>>
>> Do you have any quick hints ?

So the fix is real simple, try this extra diff:

diff --git a/drivers/gpu/drm/bridge/tc358767.c 
b/drivers/gpu/drm/bridge/tc358767.c
index 32639865fea07..5d76285dcf245 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1629,7 +1629,7 @@ static int tc_dpi_atomic_check(struct drm_bridge 
*bridge,
         int ret;

         ret = tc_pxl_pll_calc(tc, clk_get_rate(tc->refclk),
-                             crtc_state->adjusted_mode.clock * 1000,
+                             crtc_state->mode.clock * 1000,
                               &adjusted_clock, NULL);
         if (ret)
                 return ret;
@@ -1653,7 +1653,7 @@ static int tc_edp_atomic_check(struct drm_bridge 
*bridge,
         int ret;

         ret = tc_pxl_pll_calc(tc, clk_get_rate(tc->refclk),
-                             crtc_state->adjusted_mode.clock * 1000,
+                             crtc_state->mode.clock * 1000,
                               &adjusted_clock, NULL);
         if (ret)
                 return ret;

> No, sorry. I'm not sure about those VFIFO overruns/underruns you mentioned
> in the commit message. Does this maybe only apply to DPI input?

No, this actually happens with DSI input in both DPI and (e)DP output 
modes, it is only really well visible in some resolutions it seems.

> At least for 148.5MHz (1080p) apparently it is not possible to that
> exact clock anyway.

Right, which sucks, but the TC9595 datasheet explicitly states that the 
FRMSYNC mode should always be enabled (and is apparently force-enabled 
on newer bridge chips with no option to disable it) unless the Pixel 
clock are sourced from DSI clock (which is never the case with this 
driver). That's what the [1] patch does.

But messing with the HFP isn't really working for all resolutions, so 
this patch instead adjusts the input pixel clock to fit the Pixel PLL 
limits, which avoids the VFIFO issues altogether. And that makes the 
FRMSYNC mode work for all kinds of resolutions.

You might be wondering why not source pixel clock from DSI HS clock and 
disable FRMSYNC. For one thing, this would limit the ability to pick 
faster DSI HS clock and make good use of the DSI burst mode (the DSI 
link can go into LP state after transmitting entire line of pixel data 
for longer with faster DSI HS clock). The other thing is, to drive the 
Pixel PLL (not to be confused with pixel clock) from DSI HS clock, the 
DSI HS clock have to be set to specific clock rates (13*4*7=364 MHz and 
13*4*9=468 MHz), which is really limiting.

>>> BTW: Which platform are you testing on?
>>
>> MX8MP with LCDIFv3 -> DSIM -> TC9595 -> DP output.
>>
>> The TC9595 is 2nd generation (automotive?) replacement for TC358767 (1st
>> generation replacement is TC358867) .
> 
> Same here. But fail to get output on a DP monitor if I'm running from
> external refclk. Using DSICLK/4 seems necessary for some reason, but it
> is very unreliable to get a proper image.

Do you have all the patches in place ? This is what you should have:

drm/bridge: tc358767: Enable FRMSYNC timing generator
drm: bridge: samsung-dsim: Initialize bridge on attach
drm/bridge: tc358767: Reset chip again on attach
clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate
drm: lcdif: Use adjusted_mode .clock instead of .crtc_clock
drm/bridge: tc358767: Fix comment in tc_edp_mode_valid
drm/bridge: tc358767: Check if fully initialized before signalling HPD 
event via IRQ
drm/bridge: tc358767: Split tc_pxl_pll_en() into parameter calculation 
and enablement
drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct adjusted_mode clock
drm/bridge: tc358767: Drop line_pixel_subtract
drm/bridge: tc358767: Disable MIPI_DSI_CLOCK_NON_CONTINUOUS
drm/bridge: tc358767: Set LSCLK divider for SYSCLK to 1
Revert "drm/bridge: tc358767: Set default CLRSIPO count"
drm/bridge: tc358767: Add configurable default preemphasis

I only use external refclk, at 26 MHz.

> Which display are you using? Do you mind sharing your DT?

HP LA2405wg , ancient 1920x1200 one.

Relevant parts are below:

// This is in I2C controller node, it actually is modified
// arch/arm64/boot/dts/freescale/imx8mp-dhcom-som.dtsi
tc_bridge: bridge@f {
         compatible = "toshiba,tc9595", "toshiba,tc358767";
         pinctrl-names = "default";
         pinctrl-0 = <&pinctrl_tc9595>;
         reg = <0xf>;
         clock-names = "ref";
         clocks = <&clk IMX8MP_CLK_CLKOUT2>;
         assigned-clocks = <&clk IMX8MP_CLK_CLKOUT2_SEL>,
                           <&clk IMX8MP_CLK_CLKOUT2>,
                           <&clk IMX8MP_AUDIO_PLL2_OUT>;
         assigned-clock-parents = <&clk IMX8MP_AUDIO_PLL2_OUT>;
         assigned-clock-rates = <26000000>, <26000000>, <416000000>;
         reset-gpios = <&gpio4 1 GPIO_ACTIVE_HIGH>;
         status = "okay";

         ports {
                 #address-cells = <1>;
                 #size-cells = <0>;

                 port@0 {
                         reg = <0>;

                         tc_bridge_in: endpoint {
                                 data-lanes = <1 2 3 4>;
                                 remote-endpoint = <&dsi_out>;
                         };
                 };

                 port@2 { // This is optional
                         reg = <2>;
                         toshiba,pre-emphasis = /bits/ 8 <1 1>;
                 };
         };
};

// 1 GHz burst clock is the maximum the bridge can do
&mipi_dsi {
         samsung,burst-clock-frequency = <1000000000>;
         samsung,esc-clock-frequency = <10000000>;
         status = "okay";

         ports {
                 port@1 {
                         reg = <1>;

                         dsi_out: endpoint {
                                 data-lanes = <1 2 3 4>;
                                 remote-endpoint = <&tc_bridge_in>;
                         };
                 };
         };
};

// This is to let LCDIF configure the pixel clock,
// it removes the fixed Video PLL configuration from DT
&media_blk_ctrl {
        assigned-clocks = <&clk IMX8MP_CLK_MEDIA_AXI>,
                          <&clk IMX8MP_CLK_MEDIA_APB>,
                          <&clk IMX8MP_CLK_MEDIA_DISP1_PIX>,
                          <&clk IMX8MP_CLK_MEDIA_DISP2_PIX>;
        assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>,
                                 <&clk IMX8MP_SYS_PLL1_800M>,
                                 <&clk IMX8MP_VIDEO_PLL1_OUT>,
                                 <&clk IMX8MP_SYS_PLL3_OUT>;
        assigned-clock-rates = <500000000>, <200000000>,
                               <0>, <0>;
};

Re: [PATCH 3/6] drm/bridge: tc358767: Drop line_pixel_subtract

[Marek Vasut]

On 6/4/24 1:12 PM, Alexander Stein wrote:
> Hi Marek,

Hi,

> Am Montag, 3. Juni 2024, 23:25:43 CEST schrieb Marek Vasut:
>> On 6/3/24 2:18 PM, Alexander Stein wrote:
>>> Hi Marek,
>>
>> Hi,
>>
>>> Am Freitag, 31. Mai 2024, 22:39:49 CEST schrieb Marek Vasut:
>>>> This line_pixel_subtract is no longer needed now that the bridge can
>>>> request and obtain specific pixel clock on input to the bridge, with
>>>> clock frequency that matches the Pixel PLL frequency.
>>>>
>>>> The line_pixel_subtract is now always 0, so drop it entirely.
>>>>
>>>> The line_pixel_subtract was not reliable as it never worked when the
>>>> Pixel PLL and input clock were off just so that the required amount
>>>> of pixels to subtract would not be whole integer.
>>>
>>> I think this is based on [1], no?
>>
>> It is.
> 
> Thanks for confirmation.
> 
>>> I was wondering because it was not stated.
>>
>> I thought [1] was already applied, but it seems it was only RBd.
>>
>> I can either apply [1] and then add this on top, so the two commits can
>> be reverted separately if this one breaks something, or squash [1] into
>> this patch and send V2. Which one do you prefer ?
>>
>> The [1] fixes a real nasty issue with DPI output which causes visible
>> image corruption, so I would like to have [1] in, but it is obviously
>> not perfect.
> 
> I can't use DPI anyway, but if [1] actually fixes something for DPI
> I'm okay with having [1] in, which gets mostly reverted in this series.
> But that's up to the maintainers.

It is the FRMSYNC enablement (as suggested by the TC9595 datasheet) 
which fixes the issue. This just makes FRMSYNC work well with other modes.

Re: [PATCH 2/6] drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct adjusted_mode clock

[Alexander Stein]

Hi Marek,

Am Dienstag, 4. Juni 2024, 18:17:53 CEST schrieb Marek Vasut:
> On 6/4/24 1:35 PM, Alexander Stein wrote:
> > Hi Marek,
> 
> Hi,
> 
> > Am Montag, 3. Juni 2024, 23:27:34 CEST schrieb Marek Vasut:
> >> On 6/3/24 2:45 PM, Alexander Stein wrote:
> >>
> >> Hi,
> >>
> >>>> @@ -1631,6 +1643,18 @@ static int tc_edp_atomic_check(struct drm_bridge *bridge,
> >>>>    			       struct drm_crtc_state *crtc_state,
> >>>>    			       struct drm_connector_state *conn_state)
> >>>>    {
> >>>> +	struct tc_data *tc = bridge_to_tc(bridge);
> >>>> +	int adjusted_clock = 0;
> >>>> +	int ret;
> >>>> +
> >>>> +	ret = tc_pxl_pll_calc(tc, clk_get_rate(tc->refclk),
> >>>> +			      crtc_state->adjusted_mode.clock * 1000,
> >>>> +			      &adjusted_clock, NULL);
> >>>> +	if (ret)
> >>>> +		return ret;
> >>>> +
> >>>> +	crtc_state->adjusted_mode.clock = adjusted_clock / 1000;
> >>>
> >>> This is prone to rounding errors. Debug output in my case:
> >>>> [   16.007127] tc358767 1-000f: enable video stream
> >>>> [   16.007148] tc358767 1-000f: PLL: requested 148500000 pixelclock, ref 26000000
> >>>> [   16.007163] tc358767 1-000f: PLL: got 147333333, delta -1166667
> >>>> [   16.007169] tc358767 1-000f: PLL: 26000000 / 1 / 1 * 17 / 3
> >>>> [   16.027112] tc358767 1-000f: set mode 1920x1080
> >>>> [   16.027138] tc358767 1-000f: H margin 148,88 sync 44
> >>>> [   16.027144] tc358767 1-000f: V margin 36,4 sync 5
> >>>> [   16.027150] tc358767 1-000f: total: 2200x1125
> >>>> [   16.059426] tc358767 1-000f: PLL: requested 147333000 pixelclock, ref 26000000
> >>>> [   16.059455] tc358767 1-000f: PLL: got 146250000, delta -1083000
> >>>> [   16.059461] tc358767 1-000f: PLL: 26000000 / 1 / 4 * 45 / 2
> >>>> [   16.095724] tc358767 1-000f: PLL: requested 146250000 pixelclock, ref 26000000
> >>>> [   16.095739] tc358767 1-000f: PLL: got 146250000, delta 0
> >>>> [   16.095745] tc358767 1-000f: PLL: 26000000 / 1 / 4 * 45 / 2
> >>>
> >>> The accuracy degrades with each call, until a full kHz frequency is reached,
> >>> because drm_display_mode.clock only accounts for kHz, but the PLL
> >>> calculation takes Hz into account.
> >>
> >> Hmmmmm, I need to take a closer look at this one.
> >>
> >> Do you have any quick hints ?
> 
> So the fix is real simple, try this extra diff:
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c 
> b/drivers/gpu/drm/bridge/tc358767.c
> index 32639865fea07..5d76285dcf245 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -1629,7 +1629,7 @@ static int tc_dpi_atomic_check(struct drm_bridge 
> *bridge,
>          int ret;
> 
>          ret = tc_pxl_pll_calc(tc, clk_get_rate(tc->refclk),
> -                             crtc_state->adjusted_mode.clock * 1000,
> +                             crtc_state->mode.clock * 1000,
>                                &adjusted_clock, NULL);
>          if (ret)
>                  return ret;
> @@ -1653,7 +1653,7 @@ static int tc_edp_atomic_check(struct drm_bridge 
> *bridge,
>          int ret;
> 
>          ret = tc_pxl_pll_calc(tc, clk_get_rate(tc->refclk),
> -                             crtc_state->adjusted_mode.clock * 1000,
> +                             crtc_state->mode.clock * 1000,
>                                &adjusted_clock, NULL);
>          if (ret)
>                  return ret;
> 

Ah, right. That seems simple. But I noticed another thing:
The TC9595 PLL is configured to 147333333 while the lcdif configures
the display clock to 147333000, the value actually stored in
adjusted_mode.clock. I do not know if this small difference can be neglected.

> > No, sorry. I'm not sure about those VFIFO overruns/underruns you mentioned
> > in the commit message. Does this maybe only apply to DPI input?
> 
> No, this actually happens with DSI input in both DPI and (e)DP output 
> modes, it is only really well visible in some resolutions it seems.

Ah, i see.

> > At least for 148.5MHz (1080p) apparently it is not possible to that
> > exact clock anyway.
> 
> Right, which sucks, but the TC9595 datasheet explicitly states that the 
> FRMSYNC mode should always be enabled (and is apparently force-enabled 
> on newer bridge chips with no option to disable it) unless the Pixel 
> clock are sourced from DSI clock (which is never the case with this 
> driver). That's what the [1] patch does.
> 
> But messing with the HFP isn't really working for all resolutions, so 
> this patch instead adjusts the input pixel clock to fit the Pixel PLL 
> limits, which avoids the VFIFO issues altogether. And that makes the 
> FRMSYNC mode work for all kinds of resolutions.

I can't tell if VFIFO issue arise here, because I'm currently trying to
get a reliable and stable output for DP. The documentation is somewhat
limited, but FRAMSYNC seems almost necessary in any case, unless you
utilize DSI bus 100% for this device to get the correct display timings
using DSI packets.

> You might be wondering why not source pixel clock from DSI HS clock and 
> disable FRMSYNC. For one thing, this would limit the ability to pick 
> faster DSI HS clock and make good use of the DSI burst mode (the DSI 
> link can go into LP state after transmitting entire line of pixel data 
> for longer with faster DSI HS clock). The other thing is, to drive the 
> Pixel PLL (not to be confused with pixel clock) from DSI HS clock, the 
> DSI HS clock have to be set to specific clock rates (13*4*7=364 MHz and 
> 13*4*9=468 MHz), which is really limiting.

This is not mentioned in the datasheet at all, but just in a small note
in the calculation sheet, without any description. On a different platform
DSI HS clock running at 445.5MHz seems also possible, even if unsupported.

> >>> BTW: Which platform are you testing on?
> >>
> >> MX8MP with LCDIFv3 -> DSIM -> TC9595 -> DP output.
> >>
> >> The TC9595 is 2nd generation (automotive?) replacement for TC358767 (1st
> >> generation replacement is TC358867) .
> > 
> > Same here. But fail to get output on a DP monitor if I'm running from
> > external refclk. Using DSICLK/4 seems necessary for some reason, but it
> > is very unreliable to get a proper image.
> 
> Do you have all the patches in place ? This is what you should have:
> 
> drm/bridge: tc358767: Enable FRMSYNC timing generator
> drm: bridge: samsung-dsim: Initialize bridge on attach
> drm/bridge: tc358767: Reset chip again on attach
> clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate
> drm: lcdif: Use adjusted_mode .clock instead of .crtc_clock
> drm/bridge: tc358767: Fix comment in tc_edp_mode_valid
> drm/bridge: tc358767: Check if fully initialized before signalling HPD 
> event via IRQ
> drm/bridge: tc358767: Split tc_pxl_pll_en() into parameter calculation 
> and enablement
> drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct adjusted_mode clock
> drm/bridge: tc358767: Drop line_pixel_subtract
> drm/bridge: tc358767: Disable MIPI_DSI_CLOCK_NON_CONTINUOUS
> drm/bridge: tc358767: Set LSCLK divider for SYSCLK to 1
> Revert "drm/bridge: tc358767: Set default CLRSIPO count"
> drm/bridge: tc358767: Add configurable default preemphasis

Thanks, I was missing the lcdif and clk-imx8mp patches. I saw them separately
on the mailing list, but didn't realize would need them.

> I only use external refclk, at 26 MHz.

Same for me, difference is I'm using a crystal oscillator instead of
CCM_CLKOUT. And yes, this is the TQ platform TQMa8MPxL/MBa8MPxL.

> > Which display are you using? Do you mind sharing your DT?
> 
> HP LA2405wg , ancient 1920x1200 one.

ASUS PB238, 1920x1080 here.

> Relevant parts are below:
> 
> // This is in I2C controller node, it actually is modified
> // arch/arm64/boot/dts/freescale/imx8mp-dhcom-som.dtsi
> tc_bridge: bridge@f {
>          compatible = "toshiba,tc9595", "toshiba,tc358767";
>          pinctrl-names = "default";
>          pinctrl-0 = <&pinctrl_tc9595>;
>          reg = <0xf>;
>          clock-names = "ref";
>          clocks = <&clk IMX8MP_CLK_CLKOUT2>;
>          assigned-clocks = <&clk IMX8MP_CLK_CLKOUT2_SEL>,
>                            <&clk IMX8MP_CLK_CLKOUT2>,
>                            <&clk IMX8MP_AUDIO_PLL2_OUT>;
>          assigned-clock-parents = <&clk IMX8MP_AUDIO_PLL2_OUT>;
>          assigned-clock-rates = <26000000>, <26000000>, <416000000>;
>          reset-gpios = <&gpio4 1 GPIO_ACTIVE_HIGH>;
>          status = "okay";
> 
>          ports {
>                  #address-cells = <1>;
>                  #size-cells = <0>;
> 
>                  port@0 {
>                          reg = <0>;
> 
>                          tc_bridge_in: endpoint {
>                                  data-lanes = <1 2 3 4>;
>                                  remote-endpoint = <&dsi_out>;
>                          };
>                  };
> 
>                  port@2 { // This is optional
>                          reg = <2>;
>                          toshiba,pre-emphasis = /bits/ 8 <1 1>;
>                  };
>          };
> };
> 
> // 1 GHz burst clock is the maximum the bridge can do
> &mipi_dsi {
>          samsung,burst-clock-frequency = <1000000000>;
>          samsung,esc-clock-frequency = <10000000>;
>          status = "okay";
> 
>          ports {
>                  port@1 {
>                          reg = <1>;
> 
>                          dsi_out: endpoint {
>                                  data-lanes = <1 2 3 4>;
>                                  remote-endpoint = <&tc_bridge_in>;
>                          };
>                  };
>          };
> };

Thanks for sharing, despite the fixed-clock and IMX8MP_CLK_CLKOUT2 this
looks very much the same. Unfortunately I get an output on DP just sometimes.
As Bit 0 in register 0x464 is not cleared, I suspect the bridge is not
recognizing DSI (VSYNC) packets properly :(
At some point this bridge is lenient, but picky otherwise.

> // This is to let LCDIF configure the pixel clock,
> // it removes the fixed Video PLL configuration from DT
> &media_blk_ctrl {
>         assigned-clocks = <&clk IMX8MP_CLK_MEDIA_AXI>,
>                           <&clk IMX8MP_CLK_MEDIA_APB>,
>                           <&clk IMX8MP_CLK_MEDIA_DISP1_PIX>,
>                           <&clk IMX8MP_CLK_MEDIA_DISP2_PIX>;
>         assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>,
>                                  <&clk IMX8MP_SYS_PLL1_800M>,
>                                  <&clk IMX8MP_VIDEO_PLL1_OUT>,
>                                  <&clk IMX8MP_SYS_PLL3_OUT>;
>         assigned-clock-rates = <500000000>, <200000000>,
>                                <0>, <0>;
> };

I get the intention, but this might change once you want to enable ISP/DWE.
But that is a different matter for now.

Best regards,
Alexander

-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH 2/6] drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct adjusted_mode clock

[Marek Vasut]

On 6/5/24 12:52 PM, Alexander Stein wrote:
> Hi Marek,

Hi,

>>>>> The accuracy degrades with each call, until a full kHz frequency is reached,
>>>>> because drm_display_mode.clock only accounts for kHz, but the PLL
>>>>> calculation takes Hz into account.
>>>>
>>>> Hmmmmm, I need to take a closer look at this one.
>>>>
>>>> Do you have any quick hints ?
>>
>> So the fix is real simple, try this extra diff:
>>
>> diff --git a/drivers/gpu/drm/bridge/tc358767.c
>> b/drivers/gpu/drm/bridge/tc358767.c
>> index 32639865fea07..5d76285dcf245 100644
>> --- a/drivers/gpu/drm/bridge/tc358767.c
>> +++ b/drivers/gpu/drm/bridge/tc358767.c
>> @@ -1629,7 +1629,7 @@ static int tc_dpi_atomic_check(struct drm_bridge
>> *bridge,
>>           int ret;
>>
>>           ret = tc_pxl_pll_calc(tc, clk_get_rate(tc->refclk),
>> -                             crtc_state->adjusted_mode.clock * 1000,
>> +                             crtc_state->mode.clock * 1000,
>>                                 &adjusted_clock, NULL);
>>           if (ret)
>>                   return ret;
>> @@ -1653,7 +1653,7 @@ static int tc_edp_atomic_check(struct drm_bridge
>> *bridge,
>>           int ret;
>>
>>           ret = tc_pxl_pll_calc(tc, clk_get_rate(tc->refclk),
>> -                             crtc_state->adjusted_mode.clock * 1000,
>> +                             crtc_state->mode.clock * 1000,
>>                                 &adjusted_clock, NULL);
>>           if (ret)
>>                   return ret;
>>
> 
> Ah, right. That seems simple. But I noticed another thing:
> The TC9595 PLL is configured to 147333333 while the lcdif configures
> the display clock to 147333000, the value actually stored in
> adjusted_mode.clock. I do not know if this small difference can be neglected.

Good point, please see (*) below.

>>> No, sorry. I'm not sure about those VFIFO overruns/underruns you mentioned
>>> in the commit message. Does this maybe only apply to DPI input?
>>
>> No, this actually happens with DSI input in both DPI and (e)DP output
>> modes, it is only really well visible in some resolutions it seems.
> 
> Ah, i see.
> 
>>> At least for 148.5MHz (1080p) apparently it is not possible to that
>>> exact clock anyway.
>>
>> Right, which sucks, but the TC9595 datasheet explicitly states that the
>> FRMSYNC mode should always be enabled (and is apparently force-enabled
>> on newer bridge chips with no option to disable it) unless the Pixel
>> clock are sourced from DSI clock (which is never the case with this
>> driver). That's what the [1] patch does.
>>
>> But messing with the HFP isn't really working for all resolutions, so
>> this patch instead adjusts the input pixel clock to fit the Pixel PLL
>> limits, which avoids the VFIFO issues altogether. And that makes the
>> FRMSYNC mode work for all kinds of resolutions.
> 
> I can't tell if VFIFO issue arise here, because I'm currently trying to
> get a reliable and stable output for DP.

What kind of problem do you observe on your side exactly ? (I think the 
answer to this is at the end). I've spent quite a while on this, so 
maybe I ran into that before.

> The documentation is somewhat
> limited, but FRAMSYNC seems almost necessary in any case, unless you
> utilize DSI bus 100% for this device to get the correct display timings
> using DSI packets.

That's not quite what it says.

I don't know what kind of datasheet you have and for which exact bridge, 
but look at Section 4, block diagram.

The FRMSYNC operates on the LS_Clk domain side, right of V_FiFo . It 
reads lines from the VFIFO and sends them out of the DP as DP packets. 
As long as the input into the VFIFO delivers the lines such that the 
VFIFO does not overflow or underflow, everything is fine.

The DSI side starts to write line into the VFIFO on HSS (Hsync start), 
see "DSI Packets for Video Transmission". HSE (Hsync end) packet is 
ignored, see "Video Transmission" . Two consecutive HSS must be sent 
with the same time between the two as time between two lines configured 
into FRMSYNC timing generator.

However, I _think_ (*) if the time between two HSS is slightly longer 
than what the FRMSYNC expects, the FRMSYNC stretches the HFP slightly 
for each frame and we won't run into any problems (see 4.12 Clocks and 
search for keyword "approximated", they talk about close approximation 
there).

>> You might be wondering why not source pixel clock from DSI HS clock and
>> disable FRMSYNC. For one thing, this would limit the ability to pick
>> faster DSI HS clock and make good use of the DSI burst mode (the DSI
>> link can go into LP state after transmitting entire line of pixel data
>> for longer with faster DSI HS clock). The other thing is, to drive the
>> Pixel PLL (not to be confused with pixel clock) from DSI HS clock, the
>> DSI HS clock have to be set to specific clock rates (13*4*7=364 MHz and
>> 13*4*9=468 MHz), which is really limiting.

btw. those 13 MHz match one of RefClk input options, the *4 is from 
HSBY4 divider and 7 and 9 come from MODE[1] strap option dividers.

> This is not mentioned in the datasheet at all, but just in a small note
> in the calculation sheet, without any description. On a different platform
> DSI HS clock running at 445.5MHz seems also possible, even if unsupported.

You can use arbitrary DSI HS clock as long as you don't derive RefClk 
from DSI HS clock (and RefClk feeds Pixel PLL).

>>>>> BTW: Which platform are you testing on?
>>>>
>>>> MX8MP with LCDIFv3 -> DSIM -> TC9595 -> DP output.
>>>>
>>>> The TC9595 is 2nd generation (automotive?) replacement for TC358767 (1st
>>>> generation replacement is TC358867) .
>>>
>>> Same here. But fail to get output on a DP monitor if I'm running from
>>> external refclk. Using DSICLK/4 seems necessary for some reason, but it
>>> is very unreliable to get a proper image.
>>
>> Do you have all the patches in place ? This is what you should have:
>>
>> drm/bridge: tc358767: Enable FRMSYNC timing generator
>> drm: bridge: samsung-dsim: Initialize bridge on attach
>> drm/bridge: tc358767: Reset chip again on attach
>> clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate
>> drm: lcdif: Use adjusted_mode .clock instead of .crtc_clock
>> drm/bridge: tc358767: Fix comment in tc_edp_mode_valid
>> drm/bridge: tc358767: Check if fully initialized before signalling HPD
>> event via IRQ
>> drm/bridge: tc358767: Split tc_pxl_pll_en() into parameter calculation
>> and enablement
>> drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct adjusted_mode clock
>> drm/bridge: tc358767: Drop line_pixel_subtract
>> drm/bridge: tc358767: Disable MIPI_DSI_CLOCK_NON_CONTINUOUS
>> drm/bridge: tc358767: Set LSCLK divider for SYSCLK to 1
>> Revert "drm/bridge: tc358767: Set default CLRSIPO count"
>> drm/bridge: tc358767: Add configurable default preemphasis
> 
> Thanks, I was missing the lcdif and clk-imx8mp patches. I saw them separately
> on the mailing list, but didn't realize would need them.
> 
>> I only use external refclk, at 26 MHz.
> 
> Same for me, difference is I'm using a crystal oscillator instead of
> CCM_CLKOUT. And yes, this is the TQ platform TQMa8MPxL/MBa8MPxL.

I also have a SoM modified to use Xtal, but it seems unnecessary, the 
CCM output saves on BoM too.

[...]

> Thanks for sharing, despite the fixed-clock and IMX8MP_CLK_CLKOUT2 this
> looks very much the same. Unfortunately I get an output on DP just sometimes.
> As Bit 0 in register 0x464 is not cleared, I suspect the bridge is not
> recognizing DSI (VSYNC) packets properly :(
> At some point this bridge is lenient, but picky otherwise.

You did strap the bridge such that it sources RefClk from the Xtal, 
right (MODE[0] = 0 -> RefClk)? I haven't seen VFUEN[0] lock up like that 
before.

You could try and force the bridge to generate test pattern instead of 
DSI_RX, edit the tc358767.c -> locate tc_dsi_rx_enable() -> locate 
tc_test_pattern -> make sure the branch option value |= 
DP0_VIDSRC_COLOR_BAR is activated . Rebuild and retest .

This will allow you to validate the TC358767<->DP connection separately 
from the DSI<->TC358767 connection . This is what I did when I was 
debugging DSI issues on another board with TC9595 . Once you are sure 
one side of the connection is stable, you can focus on the other side.

Also, with the COLOR_BAR mode in place, try experimenting with the DP 
preemphasis tuning and possibly the initial differential voltage tuning 
(it is in the same registers as the preemphasis). Maybe you have link 
quality issues and the link is borderline, it does train and link up, 
but fails right after. Increasing the preemphasis and differential 
voltage might help stabilize it.

Also, I use these to fiddle with the TC9595 (in my case) DP timing 
registers, the first one reads them all out, the second one writes them. 
Aadjust values as needed, the values below are from whatever experiment 
so they are likely invalid. Make specific note of the last 0x01 on the 
second line, that's the VFUEN update:
$ i2ctransfer -f -y 2 w2@0xf 0x4 0x50 r24
$ i2ctransfer -f -y 2 w26@0xf 0x4 0x50   0x10 0x01 0x50 0x01   0x20 0x00 
0x50 0x00   0x80 0x07 0x20 0x00   0x06 0x00 0x1a 0x00   0xb0 0x04 0x03 
0x00   0x01 0x00 0x00 0x00

>> // This is to let LCDIF configure the pixel clock,
>> // it removes the fixed Video PLL configuration from DT
>> &media_blk_ctrl {
>>          assigned-clocks = <&clk IMX8MP_CLK_MEDIA_AXI>,
>>                            <&clk IMX8MP_CLK_MEDIA_APB>,
>>                            <&clk IMX8MP_CLK_MEDIA_DISP1_PIX>,
>>                            <&clk IMX8MP_CLK_MEDIA_DISP2_PIX>;
>>          assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>,
>>                                   <&clk IMX8MP_SYS_PLL1_800M>,
>>                                   <&clk IMX8MP_VIDEO_PLL1_OUT>,
>>                                   <&clk IMX8MP_SYS_PLL3_OUT>;
>>          assigned-clock-rates = <500000000>, <200000000>,
>>                                 <0>, <0>;
>> };
> 
> I get the intention, but this might change once you want to enable ISP/DWE.
> But that is a different matter for now.

ACK

[...]

Re: [PATCH 2/6] drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct adjusted_mode clock

[Alexander Stein]

Hi Marek,

Am Mittwoch, 5. Juni 2024, 18:25:13 CEST schrieb Marek Vasut:
> On 6/5/24 12:52 PM, Alexander Stein wrote:
> > Hi Marek,
> 
> Hi,
> 
> [snip]
> > Ah, right. That seems simple. But I noticed another thing:
> > The TC9595 PLL is configured to 147333333 while the lcdif configures
> > the display clock to 147333000, the value actually stored in
> > adjusted_mode.clock. I do not know if this small difference can be neglected.
> 
> Good point, please see (*) below.
> 
> >>> No, sorry. I'm not sure about those VFIFO overruns/underruns you mentioned
> >>> in the commit message. Does this maybe only apply to DPI input?
> >>
> >> No, this actually happens with DSI input in both DPI and (e)DP output
> >> modes, it is only really well visible in some resolutions it seems.
> > 
> > Ah, i see.
> > 
> >>> At least for 148.5MHz (1080p) apparently it is not possible to that
> >>> exact clock anyway.
> >>
> >> Right, which sucks, but the TC9595 datasheet explicitly states that the
> >> FRMSYNC mode should always be enabled (and is apparently force-enabled
> >> on newer bridge chips with no option to disable it) unless the Pixel
> >> clock are sourced from DSI clock (which is never the case with this
> >> driver). That's what the [1] patch does.
> >>
> >> But messing with the HFP isn't really working for all resolutions, so
> >> this patch instead adjusts the input pixel clock to fit the Pixel PLL
> >> limits, which avoids the VFIFO issues altogether. And that makes the
> >> FRMSYNC mode work for all kinds of resolutions.
> > 
> > I can't tell if VFIFO issue arise here, because I'm currently trying to
> > get a reliable and stable output for DP.
> 
> What kind of problem do you observe on your side exactly ? (I think the 
> answer to this is at the end). I've spent quite a while on this, so 
> maybe I ran into that before.

DP output either works as expected or not at all. The monitor stays in
sleep mode then. I only notice that VFUEN is not cleared in this case.

> > The documentation is somewhat
> > limited, but FRAMSYNC seems almost necessary in any case, unless you
> > utilize DSI bus 100% for this device to get the correct display timings
> > using DSI packets.
> 
> That's not quite what it says.
> 
> I don't know what kind of datasheet you have and for which exact bridge, 
> but look at Section 4, block diagram.

I have the datasheet 0.1 and 1.1 for TC9595 available. Also the
TC358767A_867XBG v37 spreadsheet.

> The FRMSYNC operates on the LS_Clk domain side, right of V_FiFo . It 
> reads lines from the VFIFO and sends them out of the DP as DP packets. 
> As long as the input into the VFIFO delivers the lines such that the 
> VFIFO does not overflow or underflow, everything is fine.
> 
> The DSI side starts to write line into the VFIFO on HSS (Hsync start), 
> see "DSI Packets for Video Transmission". HSE (Hsync end) packet is 
> ignored, see "Video Transmission" . Two consecutive HSS must be sent 
> with the same time between the two as time between two lines configured 
> into FRMSYNC timing generator.
> 
> However, I _think_ (*) if the time between two HSS is slightly longer 
> than what the FRMSYNC expects, the FRMSYNC stretches the HFP slightly 
> for each frame and we won't run into any problems (see 4.12 Clocks and 
> search for keyword "approximated", they talk about close approximation 
> there).

Which datasheet do you have available? It might just be a typo, but the
Clocks section is 4.13 in my case.

> >> You might be wondering why not source pixel clock from DSI HS clock and
> >> disable FRMSYNC. For one thing, this would limit the ability to pick
> >> faster DSI HS clock and make good use of the DSI burst mode (the DSI
> >> link can go into LP state after transmitting entire line of pixel data
> >> for longer with faster DSI HS clock). The other thing is, to drive the
> >> Pixel PLL (not to be confused with pixel clock) from DSI HS clock, the
> >> DSI HS clock have to be set to specific clock rates (13*4*7=364 MHz and
> >> 13*4*9=468 MHz), which is really limiting.
> 
> btw. those 13 MHz match one of RefClk input options, the *4 is from 
> HSBY4 divider and 7 and 9 come from MODE[1] strap option dividers.
> 
> > This is not mentioned in the datasheet at all, but just in a small note
> > in the calculation sheet, without any description. On a different platform
> > DSI HS clock running at 445.5MHz seems also possible, even if unsupported.
> 
> You can use arbitrary DSI HS clock as long as you don't derive RefClk 
> from DSI HS clock (and RefClk feeds Pixel PLL).

Yes, that's what I would expect as well, but that other platform was
configured to derive RefClk from DSI/4.

> >>>>> BTW: Which platform are you testing on?
> >>>>
> >>>> MX8MP with LCDIFv3 -> DSIM -> TC9595 -> DP output.
> >>>>
> >>>> The TC9595 is 2nd generation (automotive?) replacement for TC358767 (1st
> >>>> generation replacement is TC358867) .
> >>>
> >>> Same here. But fail to get output on a DP monitor if I'm running from
> >>> external refclk. Using DSICLK/4 seems necessary for some reason, but it
> >>> is very unreliable to get a proper image.
> >>
> >> Do you have all the patches in place ? This is what you should have:
> >>
> >> drm/bridge: tc358767: Enable FRMSYNC timing generator
> >> drm: bridge: samsung-dsim: Initialize bridge on attach
> >> drm/bridge: tc358767: Reset chip again on attach
> >> clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate
> >> drm: lcdif: Use adjusted_mode .clock instead of .crtc_clock
> >> drm/bridge: tc358767: Fix comment in tc_edp_mode_valid
> >> drm/bridge: tc358767: Check if fully initialized before signalling HPD
> >> event via IRQ
> >> drm/bridge: tc358767: Split tc_pxl_pll_en() into parameter calculation
> >> and enablement
> >> drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct adjusted_mode clock
> >> drm/bridge: tc358767: Drop line_pixel_subtract
> >> drm/bridge: tc358767: Disable MIPI_DSI_CLOCK_NON_CONTINUOUS
> >> drm/bridge: tc358767: Set LSCLK divider for SYSCLK to 1
> >> Revert "drm/bridge: tc358767: Set default CLRSIPO count"
> >> drm/bridge: tc358767: Add configurable default preemphasis
> > 
> > Thanks, I was missing the lcdif and clk-imx8mp patches. I saw them separately
> > on the mailing list, but didn't realize would need them.
> > 
> >> I only use external refclk, at 26 MHz.
> > 
> > Same for me, difference is I'm using a crystal oscillator instead of
> > CCM_CLKOUT. And yes, this is the TQ platform TQMa8MPxL/MBa8MPxL.
> 
> I also have a SoM modified to use Xtal, but it seems unnecessary, the 
> CCM output saves on BoM too.

That's right, but Xtal is what I have right now. A modification for
CCM_CLKOUT wouldbe possible if clock drift is an issue here.

> [...]
> 
> > Thanks for sharing, despite the fixed-clock and IMX8MP_CLK_CLKOUT2 this
> > looks very much the same. Unfortunately I get an output on DP just sometimes.
> > As Bit 0 in register 0x464 is not cleared, I suspect the bridge is not
> > recognizing DSI (VSYNC) packets properly :(
> > At some point this bridge is lenient, but picky otherwise.
> 
> You did strap the bridge such that it sources RefClk from the Xtal, 
> right (MODE[0] = 0 -> RefClk)? I haven't seen VFUEN[0] lock up like that 
> before.

Yes, MODE[0] is pulled down. Otherwise I2C is not even possible.
I've tried using MODE[0]=1 with a fixed DSI HS clock, but without luck.

> You could try and force the bridge to generate test pattern instead of 
> DSI_RX, edit the tc358767.c -> locate tc_dsi_rx_enable() -> locate 
> tc_test_pattern -> make sure the branch option value |= 
> DP0_VIDSRC_COLOR_BAR is activated . Rebuild and retest .
> 
> This will allow you to validate the TC358767<->DP connection separately 
> from the DSI<->TC358767 connection . This is what I did when I was 
> debugging DSI issues on another board with TC9595 . Once you are sure 
> one side of the connection is stable, you can focus on the other side.
> 
> Also, with the COLOR_BAR mode in place, try experimenting with the DP 
> preemphasis tuning and possibly the initial differential voltage tuning 
> (it is in the same registers as the preemphasis). Maybe you have link 
> quality issues and the link is borderline, it does train and link up, 
> but fails right after. Increasing the preemphasis and differential 
> voltage might help stabilize it.

Thanks, but as far I can tell I don't have any issues on the DP side.
With command line parameter 'tc358767.test=1' in place I get that test
pattern with 100% success rate. So I consider my problem on the DSI side only.
As FVUEN is cleared at the next VSYNC event I suspect the DSI timings
are (slightly) off, but unfortunately I don't have equipment to check
DSI signal quality/timings.

Are you running the DSIM host from 24MHz refclock?

> Also, I use these to fiddle with the TC9595 (in my case) DP timing 
> registers, the first one reads them all out, the second one writes them. 
> Aadjust values as needed, the values below are from whatever experiment 
> so they are likely invalid. Make specific note of the last 0x01 on the 
> second line, that's the VFUEN update:
> $ i2ctransfer -f -y 2 w2@0xf 0x4 0x50 r24
> $ i2ctransfer -f -y 2 w26@0xf 0x4 0x50   0x10 0x01 0x50 0x01   0x20 0x00 
> 0x50 0x00   0x80 0x07 0x20 0x00   0x06 0x00 0x1a 0x00   0xb0 0x04 0x03 
> 0x00   0x01 0x00 0x00 0x00
> 
> [...]

Thanks I'll keep that at hand, if they might come handy.

Best regards,
Alexander
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH 2/6] drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct adjusted_mode clock

[Marek Vasut]

On 6/6/24 12:10 PM, Alexander Stein wrote:
> Hi Marek,

Hello Alexander,

sorry for the delay.

>>>>> At least for 148.5MHz (1080p) apparently it is not possible to that
>>>>> exact clock anyway.
>>>>
>>>> Right, which sucks, but the TC9595 datasheet explicitly states that the
>>>> FRMSYNC mode should always be enabled (and is apparently force-enabled
>>>> on newer bridge chips with no option to disable it) unless the Pixel
>>>> clock are sourced from DSI clock (which is never the case with this
>>>> driver). That's what the [1] patch does.
>>>>
>>>> But messing with the HFP isn't really working for all resolutions, so
>>>> this patch instead adjusts the input pixel clock to fit the Pixel PLL
>>>> limits, which avoids the VFIFO issues altogether. And that makes the
>>>> FRMSYNC mode work for all kinds of resolutions.
>>>
>>> I can't tell if VFIFO issue arise here, because I'm currently trying to
>>> get a reliable and stable output for DP.
>>
>> What kind of problem do you observe on your side exactly ? (I think the
>> answer to this is at the end). I've spent quite a while on this, so
>> maybe I ran into that before.
> 
> DP output either works as expected or not at all. The monitor stays in
> sleep mode then. I only notice that VFUEN is not cleared in this case.

Let's continue this one at the end of this thread.

>>> The documentation is somewhat
>>> limited, but FRAMSYNC seems almost necessary in any case, unless you
>>> utilize DSI bus 100% for this device to get the correct display timings
>>> using DSI packets.
>>
>> That's not quite what it says.
>>
>> I don't know what kind of datasheet you have and for which exact bridge,
>> but look at Section 4, block diagram.
> 
> I have the datasheet 0.1 and 1.1 for TC9595 available. Also the
> TC358767A_867XBG v37 spreadsheet.

I have rev. 0.1 and rev. 1.3 datasheets for TC9595 .
I also have TC9595 DSI-to-DPI timing spreadsheet.

>> The FRMSYNC operates on the LS_Clk domain side, right of V_FiFo . It
>> reads lines from the VFIFO and sends them out of the DP as DP packets.
>> As long as the input into the VFIFO delivers the lines such that the
>> VFIFO does not overflow or underflow, everything is fine.
>>
>> The DSI side starts to write line into the VFIFO on HSS (Hsync start),
>> see "DSI Packets for Video Transmission". HSE (Hsync end) packet is
>> ignored, see "Video Transmission" . Two consecutive HSS must be sent
>> with the same time between the two as time between two lines configured
>> into FRMSYNC timing generator.
>>
>> However, I _think_ (*) if the time between two HSS is slightly longer
>> than what the FRMSYNC expects, the FRMSYNC stretches the HFP slightly
>> for each frame and we won't run into any problems (see 4.12 Clocks and
>> search for keyword "approximated", they talk about close approximation
>> there).
> 
> Which datasheet do you have available? It might just be a typo, but the
> Clocks section is 4.13 in my case.

Here I used the rev. 1.3 from 20230727 . Definitely section 4.12 in this 
datasheet, maybe they removed a chapter since ? Here is section 4 list:

- MIPI DSI Rx
- MIPI DPI Rx
- I2S Audio Rx
- DisplayPortTM Tx
- Parallel Output Mode
- GPIO Interface
- I2C Slave Interface
- Interrupt Interface
- Internal Test Pattern (Color Bar) Generator
- Reset
- Boot-Strap & State of TC9595XBG chip after Reset
- Clocks

>>>> You might be wondering why not source pixel clock from DSI HS clock and
>>>> disable FRMSYNC. For one thing, this would limit the ability to pick
>>>> faster DSI HS clock and make good use of the DSI burst mode (the DSI
>>>> link can go into LP state after transmitting entire line of pixel data
>>>> for longer with faster DSI HS clock). The other thing is, to drive the
>>>> Pixel PLL (not to be confused with pixel clock) from DSI HS clock, the
>>>> DSI HS clock have to be set to specific clock rates (13*4*7=364 MHz and
>>>> 13*4*9=468 MHz), which is really limiting.
>>
>> btw. those 13 MHz match one of RefClk input options, the *4 is from
>> HSBY4 divider and 7 and 9 come from MODE[1] strap option dividers.
>>
>>> This is not mentioned in the datasheet at all, but just in a small note
>>> in the calculation sheet, without any description. On a different platform
>>> DSI HS clock running at 445.5MHz seems also possible, even if unsupported.
>>
>> You can use arbitrary DSI HS clock as long as you don't derive RefClk
>> from DSI HS clock (and RefClk feeds Pixel PLL).
> 
> Yes, that's what I would expect as well, but that other platform was
> configured to derive RefClk from DSI/4.

I think that's the HSCKBY4 thing, the DSI HS clock are divided by 4 and 
then by either 7 or 9 to achieve 13 MHz RefClk input.

This may be achievable with some effort for DSI-to-DPI mode, but for the 
DSI-to-(e)DP mode with aux channel this is currently far away. And with 
the limitations this imposes on the DSI clock, I am not particularly 
interested in this mode of operation, sourcing the RefClk from Xtal as 
the driver assumes right now does provide much more flexibility.

>>>>>>> BTW: Which platform are you testing on?
>>>>>>
>>>>>> MX8MP with LCDIFv3 -> DSIM -> TC9595 -> DP output.
>>>>>>
>>>>>> The TC9595 is 2nd generation (automotive?) replacement for TC358767 (1st
>>>>>> generation replacement is TC358867) .
>>>>>
>>>>> Same here. But fail to get output on a DP monitor if I'm running from
>>>>> external refclk. Using DSICLK/4 seems necessary for some reason, but it
>>>>> is very unreliable to get a proper image.
>>>>
>>>> Do you have all the patches in place ? This is what you should have:
>>>>
>>>> drm/bridge: tc358767: Enable FRMSYNC timing generator
>>>> drm: bridge: samsung-dsim: Initialize bridge on attach
>>>> drm/bridge: tc358767: Reset chip again on attach
>>>> clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate
>>>> drm: lcdif: Use adjusted_mode .clock instead of .crtc_clock
>>>> drm/bridge: tc358767: Fix comment in tc_edp_mode_valid
>>>> drm/bridge: tc358767: Check if fully initialized before signalling HPD
>>>> event via IRQ
>>>> drm/bridge: tc358767: Split tc_pxl_pll_en() into parameter calculation
>>>> and enablement
>>>> drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct adjusted_mode clock
>>>> drm/bridge: tc358767: Drop line_pixel_subtract
>>>> drm/bridge: tc358767: Disable MIPI_DSI_CLOCK_NON_CONTINUOUS
>>>> drm/bridge: tc358767: Set LSCLK divider for SYSCLK to 1
>>>> Revert "drm/bridge: tc358767: Set default CLRSIPO count"
>>>> drm/bridge: tc358767: Add configurable default preemphasis
>>>
>>> Thanks, I was missing the lcdif and clk-imx8mp patches. I saw them separately
>>> on the mailing list, but didn't realize would need them.
>>>
>>>> I only use external refclk, at 26 MHz.
>>>
>>> Same for me, difference is I'm using a crystal oscillator instead of
>>> CCM_CLKOUT. And yes, this is the TQ platform TQMa8MPxL/MBa8MPxL.
>>
>> I also have a SoM modified to use Xtal, but it seems unnecessary, the
>> CCM output saves on BoM too.
> 
> That's right, but Xtal is what I have right now. A modification for
> CCM_CLKOUT wouldbe possible if clock drift is an issue here.

I think the TC9595 does have rather stringent requirements on the Xtal 
indeed, although it also seems the bridge is very tolerant about those 
input clock :)

>> [...]
>>
>>> Thanks for sharing, despite the fixed-clock and IMX8MP_CLK_CLKOUT2 this
>>> looks very much the same. Unfortunately I get an output on DP just sometimes.
>>> As Bit 0 in register 0x464 is not cleared, I suspect the bridge is not
>>> recognizing DSI (VSYNC) packets properly :(
>>> At some point this bridge is lenient, but picky otherwise.
>>
>> You did strap the bridge such that it sources RefClk from the Xtal,
>> right (MODE[0] = 0 -> RefClk)? I haven't seen VFUEN[0] lock up like that
>> before.
> 
> Yes, MODE[0] is pulled down. Otherwise I2C is not even possible.
> I've tried using MODE[0]=1 with a fixed DSI HS clock, but without luck.

You would have to have DSI clock running before releasing the bridge 
from reset. I did manage to get this working at some point with 
STM32MP15xx DSI host using crude hackage, but I don't have any interest 
in supporting this mode. All the hardware I have available has external 
RefClk.

>> You could try and force the bridge to generate test pattern instead of
>> DSI_RX, edit the tc358767.c -> locate tc_dsi_rx_enable() -> locate
>> tc_test_pattern -> make sure the branch option value |=
>> DP0_VIDSRC_COLOR_BAR is activated . Rebuild and retest .
>>
>> This will allow you to validate the TC358767<->DP connection separately
>> from the DSI<->TC358767 connection . This is what I did when I was
>> debugging DSI issues on another board with TC9595 . Once you are sure
>> one side of the connection is stable, you can focus on the other side.
>>
>> Also, with the COLOR_BAR mode in place, try experimenting with the DP
>> preemphasis tuning and possibly the initial differential voltage tuning
>> (it is in the same registers as the preemphasis). Maybe you have link
>> quality issues and the link is borderline, it does train and link up,
>> but fails right after. Increasing the preemphasis and differential
>> voltage might help stabilize it.
> 
> Thanks, but as far I can tell I don't have any issues on the DP side.
> With command line parameter 'tc358767.test=1' in place I get that test
> pattern with 100% success rate. So I consider my problem on the DSI side only.

Try tuning the CLRSIPO values up and down (probably between 3..25), does 
that help ?

> As FVUEN is cleared at the next VSYNC event I suspect the DSI timings
> are (slightly) off, but unfortunately I don't have equipment to check
> DSI signal quality/timings.

As long as the LCDIFv3 pixel clock are equal or slightly slower than 
what the TC9595 PixelPLL generates, AND, DSIM serializer has enough 
bandwidth on the DSI bus (i.e. set the bus to 1 GHz, the TC9595 DSI RX 
cannot go any faster), you should have no issues on that end.

When in doubt, try and use i2ctransfer to read out register 0x300 
repeatedly, that's DSI RX error counter register. See if the DSI error 
count increments.

> Are you running the DSIM host from 24MHz refclock?

Yes, I did not modify imx8mp.dtsi mipi_dsi node 
samsung,pll-clock-frequency = <24000000>; in any way , so that's 24 MHz 
base.

>> Also, I use these to fiddle with the TC9595 (in my case) DP timing
>> registers, the first one reads them all out, the second one writes them.
>> Aadjust values as needed, the values below are from whatever experiment
>> so they are likely invalid. Make specific note of the last 0x01 on the
>> second line, that's the VFUEN update:
>> $ i2ctransfer -f -y 2 w2@0xf 0x4 0x50 r24
>> $ i2ctransfer -f -y 2 w26@0xf 0x4 0x50   0x10 0x01 0x50 0x01   0x20 0x00
>> 0x50 0x00   0x80 0x07 0x20 0x00   0x06 0x00 0x1a 0x00   0xb0 0x04 0x03
>> 0x00   0x01 0x00 0x00 0x00
>>
>> [...]
> 
> Thanks I'll keep that at hand, if they might come handy.

Glad I could help.

Re: [PATCH 2/6] drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct adjusted_mode clock

[Marek Vasut]

On 6/11/24 6:45 PM, Marek Vasut wrote:
> On 6/6/24 12:10 PM, Alexander Stein wrote:
>> Hi Marek,
> 
> Hello Alexander,
> 
> sorry for the delay.
> 
>>>>>> At least for 148.5MHz (1080p) apparently it is not possible to that
>>>>>> exact clock anyway.
>>>>>
>>>>> Right, which sucks, but the TC9595 datasheet explicitly states that 
>>>>> the
>>>>> FRMSYNC mode should always be enabled (and is apparently force-enabled
>>>>> on newer bridge chips with no option to disable it) unless the Pixel
>>>>> clock are sourced from DSI clock (which is never the case with this
>>>>> driver). That's what the [1] patch does.
>>>>>
>>>>> But messing with the HFP isn't really working for all resolutions, so
>>>>> this patch instead adjusts the input pixel clock to fit the Pixel PLL
>>>>> limits, which avoids the VFIFO issues altogether. And that makes the
>>>>> FRMSYNC mode work for all kinds of resolutions.
>>>>
>>>> I can't tell if VFIFO issue arise here, because I'm currently trying to
>>>> get a reliable and stable output for DP.
>>>
>>> What kind of problem do you observe on your side exactly ? (I think the
>>> answer to this is at the end). I've spent quite a while on this, so
>>> maybe I ran into that before.
>>
>> DP output either works as expected or not at all. The monitor stays in
>> sleep mode then. I only notice that VFUEN is not cleared in this case.
> 
> Let's continue this one at the end of this thread.
> 
>>>> The documentation is somewhat
>>>> limited, but FRAMSYNC seems almost necessary in any case, unless you
>>>> utilize DSI bus 100% for this device to get the correct display timings
>>>> using DSI packets.
>>>
>>> That's not quite what it says.
>>>
>>> I don't know what kind of datasheet you have and for which exact bridge,
>>> but look at Section 4, block diagram.
>>
>> I have the datasheet 0.1 and 1.1 for TC9595 available. Also the
>> TC358767A_867XBG v37 spreadsheet.
> 
> I have rev. 0.1 and rev. 1.3 datasheets for TC9595 .
> I also have TC9595 DSI-to-DPI timing spreadsheet.
> 
>>> The FRMSYNC operates on the LS_Clk domain side, right of V_FiFo . It
>>> reads lines from the VFIFO and sends them out of the DP as DP packets.
>>> As long as the input into the VFIFO delivers the lines such that the
>>> VFIFO does not overflow or underflow, everything is fine.
>>>
>>> The DSI side starts to write line into the VFIFO on HSS (Hsync start),
>>> see "DSI Packets for Video Transmission". HSE (Hsync end) packet is
>>> ignored, see "Video Transmission" . Two consecutive HSS must be sent
>>> with the same time between the two as time between two lines configured
>>> into FRMSYNC timing generator.
>>>
>>> However, I _think_ (*) if the time between two HSS is slightly longer
>>> than what the FRMSYNC expects, the FRMSYNC stretches the HFP slightly
>>> for each frame and we won't run into any problems (see 4.12 Clocks and
>>> search for keyword "approximated", they talk about close approximation
>>> there).
>>
>> Which datasheet do you have available? It might just be a typo, but the
>> Clocks section is 4.13 in my case.
> 
> Here I used the rev. 1.3 from 20230727 . Definitely section 4.12 in this 
> datasheet, maybe they removed a chapter since ? Here is section 4 list:
> 
> - MIPI DSI Rx
> - MIPI DPI Rx
> - I2S Audio Rx
> - DisplayPortTM Tx
> - Parallel Output Mode
> - GPIO Interface
> - I2C Slave Interface
> - Interrupt Interface
> - Internal Test Pattern (Color Bar) Generator
> - Reset
> - Boot-Strap & State of TC9595XBG chip after Reset
> - Clocks
> 
>>>>> You might be wondering why not source pixel clock from DSI HS clock 
>>>>> and
>>>>> disable FRMSYNC. For one thing, this would limit the ability to pick
>>>>> faster DSI HS clock and make good use of the DSI burst mode (the DSI
>>>>> link can go into LP state after transmitting entire line of pixel data
>>>>> for longer with faster DSI HS clock). The other thing is, to drive the
>>>>> Pixel PLL (not to be confused with pixel clock) from DSI HS clock, the
>>>>> DSI HS clock have to be set to specific clock rates (13*4*7=364 MHz 
>>>>> and
>>>>> 13*4*9=468 MHz), which is really limiting.
>>>
>>> btw. those 13 MHz match one of RefClk input options, the *4 is from
>>> HSBY4 divider and 7 and 9 come from MODE[1] strap option dividers.
>>>
>>>> This is not mentioned in the datasheet at all, but just in a small note
>>>> in the calculation sheet, without any description. On a different 
>>>> platform
>>>> DSI HS clock running at 445.5MHz seems also possible, even if 
>>>> unsupported.
>>>
>>> You can use arbitrary DSI HS clock as long as you don't derive RefClk
>>> from DSI HS clock (and RefClk feeds Pixel PLL).
>>
>> Yes, that's what I would expect as well, but that other platform was
>> configured to derive RefClk from DSI/4.
> 
> I think that's the HSCKBY4 thing, the DSI HS clock are divided by 4 and 
> then by either 7 or 9 to achieve 13 MHz RefClk input.
> 
> This may be achievable with some effort for DSI-to-DPI mode, but for the 
> DSI-to-(e)DP mode with aux channel this is currently far away. And with 
> the limitations this imposes on the DSI clock, I am not particularly 
> interested in this mode of operation, sourcing the RefClk from Xtal as 
> the driver assumes right now does provide much more flexibility.
> 
>>>>>>>> BTW: Which platform are you testing on?
>>>>>>>
>>>>>>> MX8MP with LCDIFv3 -> DSIM -> TC9595 -> DP output.
>>>>>>>
>>>>>>> The TC9595 is 2nd generation (automotive?) replacement for 
>>>>>>> TC358767 (1st
>>>>>>> generation replacement is TC358867) .
>>>>>>
>>>>>> Same here. But fail to get output on a DP monitor if I'm running from
>>>>>> external refclk. Using DSICLK/4 seems necessary for some reason, 
>>>>>> but it
>>>>>> is very unreliable to get a proper image.
>>>>>
>>>>> Do you have all the patches in place ? This is what you should have:
>>>>>
>>>>> drm/bridge: tc358767: Enable FRMSYNC timing generator
>>>>> drm: bridge: samsung-dsim: Initialize bridge on attach
>>>>> drm/bridge: tc358767: Reset chip again on attach
>>>>> clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure 
>>>>> parent rate
>>>>> drm: lcdif: Use adjusted_mode .clock instead of .crtc_clock
>>>>> drm/bridge: tc358767: Fix comment in tc_edp_mode_valid
>>>>> drm/bridge: tc358767: Check if fully initialized before signalling HPD
>>>>> event via IRQ
>>>>> drm/bridge: tc358767: Split tc_pxl_pll_en() into parameter calculation
>>>>> and enablement
>>>>> drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct 
>>>>> adjusted_mode clock
>>>>> drm/bridge: tc358767: Drop line_pixel_subtract
>>>>> drm/bridge: tc358767: Disable MIPI_DSI_CLOCK_NON_CONTINUOUS
>>>>> drm/bridge: tc358767: Set LSCLK divider for SYSCLK to 1
>>>>> Revert "drm/bridge: tc358767: Set default CLRSIPO count"
>>>>> drm/bridge: tc358767: Add configurable default preemphasis
>>>>
>>>> Thanks, I was missing the lcdif and clk-imx8mp patches. I saw them 
>>>> separately
>>>> on the mailing list, but didn't realize would need them.
>>>>
>>>>> I only use external refclk, at 26 MHz.
>>>>
>>>> Same for me, difference is I'm using a crystal oscillator instead of
>>>> CCM_CLKOUT. And yes, this is the TQ platform TQMa8MPxL/MBa8MPxL.
>>>
>>> I also have a SoM modified to use Xtal, but it seems unnecessary, the
>>> CCM output saves on BoM too.
>>
>> That's right, but Xtal is what I have right now. A modification for
>> CCM_CLKOUT wouldbe possible if clock drift is an issue here.
> 
> I think the TC9595 does have rather stringent requirements on the Xtal 
> indeed, although it also seems the bridge is very tolerant about those 
> input clock :)
> 
>>> [...]
>>>
>>>> Thanks for sharing, despite the fixed-clock and IMX8MP_CLK_CLKOUT2 this
>>>> looks very much the same. Unfortunately I get an output on DP just 
>>>> sometimes.
>>>> As Bit 0 in register 0x464 is not cleared, I suspect the bridge is not
>>>> recognizing DSI (VSYNC) packets properly :(
>>>> At some point this bridge is lenient, but picky otherwise.
>>>
>>> You did strap the bridge such that it sources RefClk from the Xtal,
>>> right (MODE[0] = 0 -> RefClk)? I haven't seen VFUEN[0] lock up like that
>>> before.
>>
>> Yes, MODE[0] is pulled down. Otherwise I2C is not even possible.
>> I've tried using MODE[0]=1 with a fixed DSI HS clock, but without luck.
> 
> You would have to have DSI clock running before releasing the bridge 
> from reset. I did manage to get this working at some point with 
> STM32MP15xx DSI host using crude hackage, but I don't have any interest 
> in supporting this mode. All the hardware I have available has external 
> RefClk.
> 
>>> You could try and force the bridge to generate test pattern instead of
>>> DSI_RX, edit the tc358767.c -> locate tc_dsi_rx_enable() -> locate
>>> tc_test_pattern -> make sure the branch option value |=
>>> DP0_VIDSRC_COLOR_BAR is activated . Rebuild and retest .
>>>
>>> This will allow you to validate the TC358767<->DP connection separately
>>> from the DSI<->TC358767 connection . This is what I did when I was
>>> debugging DSI issues on another board with TC9595 . Once you are sure
>>> one side of the connection is stable, you can focus on the other side.
>>>
>>> Also, with the COLOR_BAR mode in place, try experimenting with the DP
>>> preemphasis tuning and possibly the initial differential voltage tuning
>>> (it is in the same registers as the preemphasis). Maybe you have link
>>> quality issues and the link is borderline, it does train and link up,
>>> but fails right after. Increasing the preemphasis and differential
>>> voltage might help stabilize it.
>>
>> Thanks, but as far I can tell I don't have any issues on the DP side.
>> With command line parameter 'tc358767.test=1' in place I get that test
>> pattern with 100% success rate. So I consider my problem on the DSI 
>> side only.
> 
> Try tuning the CLRSIPO values up and down (probably between 3..25), does 
> that help ?
> 
>> As FVUEN is cleared at the next VSYNC event I suspect the DSI timings
>> are (slightly) off, but unfortunately I don't have equipment to check
>> DSI signal quality/timings.
> 
> As long as the LCDIFv3 pixel clock are equal or slightly slower than 
> what the TC9595 PixelPLL generates, AND, DSIM serializer has enough 
> bandwidth on the DSI bus (i.e. set the bus to 1 GHz, the TC9595 DSI RX 
> cannot go any faster), you should have no issues on that end.
> 
> When in doubt, try and use i2ctransfer to read out register 0x300 
> repeatedly, that's DSI RX error counter register. See if the DSI error 
> count increments.
> 
>> Are you running the DSIM host from 24MHz refclock?
> 
> Yes, I did not modify imx8mp.dtsi mipi_dsi node 
> samsung,pll-clock-frequency = <24000000>; in any way , so that's 24 MHz 
> base.

How shall we proceed with this series ?

Re: [PATCH 2/6] drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct adjusted_mode clock

[Alexander Stein]

Hi Marek,

Am Freitag, 21. Juni 2024, 05:30:26 CEST schrieb Marek Vasut:
> On 6/11/24 6:45 PM, Marek Vasut wrote:
> > On 6/6/24 12:10 PM, Alexander Stein wrote:
> >> Hi Marek,
> > 
> > Hello Alexander,
> > 
> > sorry for the delay.
> > 
> >>>>>> At least for 148.5MHz (1080p) apparently it is not possible to that
> >>>>>> exact clock anyway.
> >>>>>
> >>>>> Right, which sucks, but the TC9595 datasheet explicitly states that 
> >>>>> the
> >>>>> FRMSYNC mode should always be enabled (and is apparently force-enabled
> >>>>> on newer bridge chips with no option to disable it) unless the Pixel
> >>>>> clock are sourced from DSI clock (which is never the case with this
> >>>>> driver). That's what the [1] patch does.
> >>>>>
> >>>>> But messing with the HFP isn't really working for all resolutions, so
> >>>>> this patch instead adjusts the input pixel clock to fit the Pixel PLL
> >>>>> limits, which avoids the VFIFO issues altogether. And that makes the
> >>>>> FRMSYNC mode work for all kinds of resolutions.
> >>>>
> >>>> I can't tell if VFIFO issue arise here, because I'm currently trying to
> >>>> get a reliable and stable output for DP.
> >>>
> >>> What kind of problem do you observe on your side exactly ? (I think the
> >>> answer to this is at the end). I've spent quite a while on this, so
> >>> maybe I ran into that before.
> >>
> >> DP output either works as expected or not at all. The monitor stays in
> >> sleep mode then. I only notice that VFUEN is not cleared in this case.
> > 
> > Let's continue this one at the end of this thread.
> > 
> >>>> The documentation is somewhat
> >>>> limited, but FRAMSYNC seems almost necessary in any case, unless you
> >>>> utilize DSI bus 100% for this device to get the correct display timings
> >>>> using DSI packets.
> >>>
> >>> That's not quite what it says.
> >>>
> >>> I don't know what kind of datasheet you have and for which exact bridge,
> >>> but look at Section 4, block diagram.
> >>
> >> I have the datasheet 0.1 and 1.1 for TC9595 available. Also the
> >> TC358767A_867XBG v37 spreadsheet.
> > 
> > I have rev. 0.1 and rev. 1.3 datasheets for TC9595 .
> > I also have TC9595 DSI-to-DPI timing spreadsheet.
> > 
> >>> The FRMSYNC operates on the LS_Clk domain side, right of V_FiFo . It
> >>> reads lines from the VFIFO and sends them out of the DP as DP packets.
> >>> As long as the input into the VFIFO delivers the lines such that the
> >>> VFIFO does not overflow or underflow, everything is fine.
> >>>
> >>> The DSI side starts to write line into the VFIFO on HSS (Hsync start),
> >>> see "DSI Packets for Video Transmission". HSE (Hsync end) packet is
> >>> ignored, see "Video Transmission" . Two consecutive HSS must be sent
> >>> with the same time between the two as time between two lines configured
> >>> into FRMSYNC timing generator.
> >>>
> >>> However, I _think_ (*) if the time between two HSS is slightly longer
> >>> than what the FRMSYNC expects, the FRMSYNC stretches the HFP slightly
> >>> for each frame and we won't run into any problems (see 4.12 Clocks and
> >>> search for keyword "approximated", they talk about close approximation
> >>> there).
> >>
> >> Which datasheet do you have available? It might just be a typo, but the
> >> Clocks section is 4.13 in my case.
> > 
> > Here I used the rev. 1.3 from 20230727 . Definitely section 4.12 in this 
> > datasheet, maybe they removed a chapter since ? Here is section 4 list:
> > 
> > - MIPI DSI Rx
> > - MIPI DPI Rx
> > - I2S Audio Rx
> > - DisplayPortTM Tx
> > - Parallel Output Mode
> > - GPIO Interface
> > - I2C Slave Interface
> > - Interrupt Interface
> > - Internal Test Pattern (Color Bar) Generator
> > - Reset
> > - Boot-Strap & State of TC9595XBG chip after Reset
> > - Clocks
> > 
> >>>>> You might be wondering why not source pixel clock from DSI HS clock 
> >>>>> and
> >>>>> disable FRMSYNC. For one thing, this would limit the ability to pick
> >>>>> faster DSI HS clock and make good use of the DSI burst mode (the DSI
> >>>>> link can go into LP state after transmitting entire line of pixel data
> >>>>> for longer with faster DSI HS clock). The other thing is, to drive the
> >>>>> Pixel PLL (not to be confused with pixel clock) from DSI HS clock, the
> >>>>> DSI HS clock have to be set to specific clock rates (13*4*7=364 MHz 
> >>>>> and
> >>>>> 13*4*9=468 MHz), which is really limiting.
> >>>
> >>> btw. those 13 MHz match one of RefClk input options, the *4 is from
> >>> HSBY4 divider and 7 and 9 come from MODE[1] strap option dividers.
> >>>
> >>>> This is not mentioned in the datasheet at all, but just in a small note
> >>>> in the calculation sheet, without any description. On a different 
> >>>> platform
> >>>> DSI HS clock running at 445.5MHz seems also possible, even if 
> >>>> unsupported.
> >>>
> >>> You can use arbitrary DSI HS clock as long as you don't derive RefClk
> >>> from DSI HS clock (and RefClk feeds Pixel PLL).
> >>
> >> Yes, that's what I would expect as well, but that other platform was
> >> configured to derive RefClk from DSI/4.
> > 
> > I think that's the HSCKBY4 thing, the DSI HS clock are divided by 4 and 
> > then by either 7 or 9 to achieve 13 MHz RefClk input.
> > 
> > This may be achievable with some effort for DSI-to-DPI mode, but for the 
> > DSI-to-(e)DP mode with aux channel this is currently far away. And with 
> > the limitations this imposes on the DSI clock, I am not particularly 
> > interested in this mode of operation, sourcing the RefClk from Xtal as 
> > the driver assumes right now does provide much more flexibility.

I don't like that mode as well, so I would be very happy to make this
reliable using RefClk from Xtal.

> >>>>>>>> BTW: Which platform are you testing on?
> >>>>>>>
> >>>>>>> MX8MP with LCDIFv3 -> DSIM -> TC9595 -> DP output.
> >>>>>>>
> >>>>>>> The TC9595 is 2nd generation (automotive?) replacement for 
> >>>>>>> TC358767 (1st
> >>>>>>> generation replacement is TC358867) .
> >>>>>>
> >>>>>> Same here. But fail to get output on a DP monitor if I'm running from
> >>>>>> external refclk. Using DSICLK/4 seems necessary for some reason, 
> >>>>>> but it
> >>>>>> is very unreliable to get a proper image.
> >>>>>
> >>>>> Do you have all the patches in place ? This is what you should have:
> >>>>>
> >>>>> drm/bridge: tc358767: Enable FRMSYNC timing generator
> >>>>> drm: bridge: samsung-dsim: Initialize bridge on attach
> >>>>> drm/bridge: tc358767: Reset chip again on attach
> >>>>> clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure 
> >>>>> parent rate
> >>>>> drm: lcdif: Use adjusted_mode .clock instead of .crtc_clock
> >>>>> drm/bridge: tc358767: Fix comment in tc_edp_mode_valid
> >>>>> drm/bridge: tc358767: Check if fully initialized before signalling HPD
> >>>>> event via IRQ
> >>>>> drm/bridge: tc358767: Split tc_pxl_pll_en() into parameter calculation
> >>>>> and enablement
> >>>>> drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct 
> >>>>> adjusted_mode clock
> >>>>> drm/bridge: tc358767: Drop line_pixel_subtract
> >>>>> drm/bridge: tc358767: Disable MIPI_DSI_CLOCK_NON_CONTINUOUS
> >>>>> drm/bridge: tc358767: Set LSCLK divider for SYSCLK to 1
> >>>>> Revert "drm/bridge: tc358767: Set default CLRSIPO count"
> >>>>> drm/bridge: tc358767: Add configurable default preemphasis
> >>>>
> >>>> Thanks, I was missing the lcdif and clk-imx8mp patches. I saw them 
> >>>> separately
> >>>> on the mailing list, but didn't realize would need them.
> >>>>
> >>>>> I only use external refclk, at 26 MHz.
> >>>>
> >>>> Same for me, difference is I'm using a crystal oscillator instead of
> >>>> CCM_CLKOUT. And yes, this is the TQ platform TQMa8MPxL/MBa8MPxL.
> >>>
> >>> I also have a SoM modified to use Xtal, but it seems unnecessary, the
> >>> CCM output saves on BoM too.
> >>
> >> That's right, but Xtal is what I have right now. A modification for
> >> CCM_CLKOUT wouldbe possible if clock drift is an issue here.
> > 
> > I think the TC9595 does have rather stringent requirements on the Xtal 
> > indeed, although it also seems the bridge is very tolerant about those 
> > input clock :)
> > 
> >>> [...]
> >>>
> >>>> Thanks for sharing, despite the fixed-clock and IMX8MP_CLK_CLKOUT2 this
> >>>> looks very much the same. Unfortunately I get an output on DP just 
> >>>> sometimes.
> >>>> As Bit 0 in register 0x464 is not cleared, I suspect the bridge is not
> >>>> recognizing DSI (VSYNC) packets properly :(
> >>>> At some point this bridge is lenient, but picky otherwise.
> >>>
> >>> You did strap the bridge such that it sources RefClk from the Xtal,
> >>> right (MODE[0] = 0 -> RefClk)? I haven't seen VFUEN[0] lock up like that
> >>> before.
> >>
> >> Yes, MODE[0] is pulled down. Otherwise I2C is not even possible.
> >> I've tried using MODE[0]=1 with a fixed DSI HS clock, but without luck.
> > 
> > You would have to have DSI clock running before releasing the bridge 
> > from reset. I did manage to get this working at some point with 
> > STM32MP15xx DSI host using crude hackage, but I don't have any interest 
> > in supporting this mode. All the hardware I have available has external 
> > RefClk.

As said above I'm not interested in using DSI clock as RefClk, whether just
for pixel PLL or even for PLL_REF.

> >>> You could try and force the bridge to generate test pattern instead of
> >>> DSI_RX, edit the tc358767.c -> locate tc_dsi_rx_enable() -> locate
> >>> tc_test_pattern -> make sure the branch option value |=
> >>> DP0_VIDSRC_COLOR_BAR is activated . Rebuild and retest .
> >>>
> >>> This will allow you to validate the TC358767<->DP connection separately
> >>> from the DSI<->TC358767 connection . This is what I did when I was
> >>> debugging DSI issues on another board with TC9595 . Once you are sure
> >>> one side of the connection is stable, you can focus on the other side.
> >>>
> >>> Also, with the COLOR_BAR mode in place, try experimenting with the DP
> >>> preemphasis tuning and possibly the initial differential voltage tuning
> >>> (it is in the same registers as the preemphasis). Maybe you have link
> >>> quality issues and the link is borderline, it does train and link up,
> >>> but fails right after. Increasing the preemphasis and differential
> >>> voltage might help stabilize it.
> >>
> >> Thanks, but as far I can tell I don't have any issues on the DP side.
> >> With command line parameter 'tc358767.test=1' in place I get that test
> >> pattern with 100% success rate. So I consider my problem on the DSI 
> >> side only.
> > 
> > Try tuning the CLRSIPO values up and down (probably between 3..25), does 
> > that help ?

I did but without luck so far to make it reliable.

> >> As FVUEN is cleared at the next VSYNC event I suspect the DSI timings
> >> are (slightly) off, but unfortunately I don't have equipment to check
> >> DSI signal quality/timings.
> > 
> > As long as the LCDIFv3 pixel clock are equal or slightly slower than 
> > what the TC9595 PixelPLL generates, AND, DSIM serializer has enough 
> > bandwidth on the DSI bus (i.e. set the bus to 1 GHz, the TC9595 DSI RX 
> > cannot go any faster), you should have no issues on that end.

I'm using samsung,burst-clock-frequency = <1000000000> so this should be
okay. That is 1080p resolution.

> > When in doubt, try and use i2ctransfer to read out register 0x300 
> > repeatedly, that's DSI RX error counter register. See if the DSI error 
> > count increments.

If the bridge is not working the registers look like this:
300: c0800000
464: 00000001

they are not changing and stay like that.

If the bridge is actually running they are like
300: c08000d3
464: 00000000

and are also not changing.

> >> Are you running the DSIM host from 24MHz refclock?
> > 
> > Yes, I did not modify imx8mp.dtsi mipi_dsi node 
> > samsung,pll-clock-frequency = <24000000>; in any way , so that's 24 MHz 
> > base.
> 
> How shall we proceed with this series ?

Would you mind rebasing and fix patch 2/6 regarding the adjusted mode?

BTW: Patch 3/6 (dropping line_pixel_subtract) is actually necessary for even
running test mode (tc358767.test=1), so this fixes an actual problem as well.

Best regards,
Alexander
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH 2/6] drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct adjusted_mode clock

[Marek Vasut]

On 6/21/24 12:32 PM, Alexander Stein wrote:

Hi,

skipping the parts where I would simply write "OK" ...

>>>> As FVUEN is cleared at the next VSYNC event I suspect the DSI timings
>>>> are (slightly) off, but unfortunately I don't have equipment to check
>>>> DSI signal quality/timings.
>>>
>>> As long as the LCDIFv3 pixel clock are equal or slightly slower than
>>> what the TC9595 PixelPLL generates, AND, DSIM serializer has enough
>>> bandwidth on the DSI bus (i.e. set the bus to 1 GHz, the TC9595 DSI RX
>>> cannot go any faster), you should have no issues on that end.
> 
> I'm using samsung,burst-clock-frequency = <1000000000> so this should be
> okay. That is 1080p resolution.

Yes, correct.

>>> When in doubt, try and use i2ctransfer to read out register 0x300
>>> repeatedly, that's DSI RX error counter register. See if the DSI error
>>> count increments.
> 
> If the bridge is not working the registers look like this:
> 300: c0800000
> 464: 00000001
> 
> they are not changing and stay like that.
> 
> If the bridge is actually running they are like
> 300: c08000d3
> 464: 00000000
> 
> and are also not changing.

Uh ... that looks like the whole chip clock tree somehow locked up .

Thinking about this, I once did force the DSIM into 24 MHz mode (there 
is PLL bypass setting, where the DSIM uses 24 MHz serializer clock 
directly for the DSI HS clock) or something close, it was enough to 
drive a low resolution panel. But the upside was, with a 200 MHz 5Gsps 
scope set to AC-coupling and 10x probe, I could discern the traffic on 
DSI data lane and decode it by hand. The nice thing is, you could 
trigger on 1V2 LP mode, so you know where the packet starts. The 
downside is, if you have multiple data lanes, the packet is spread 
across them.

You could also tweak tc_edp_atomic_check()/tc_edp_mode_valid() and force 
only low(er) resolution modes of your DP panel right from the start, so 
you wouldn't need that much DSI bandwidth. Maybe you could reach some 
mode where your equipment is enough to analyze the traffic by hand ?

>>>> Are you running the DSIM host from 24MHz refclock?
>>>
>>> Yes, I did not modify imx8mp.dtsi mipi_dsi node
>>> samsung,pll-clock-frequency = <24000000>; in any way , so that's 24 MHz
>>> base.
>>
>> How shall we proceed with this series ?
> 
> Would you mind rebasing and fix patch 2/6 regarding the adjusted mode?

I wouldn't ... and I totally forgot about that, even if I have a V2 
patch in my tree already. I'll send it out, thanks for the reminder.

> BTW: Patch 3/6 (dropping line_pixel_subtract) is actually necessary for even
> running test mode (tc358767.test=1), so this fixes an actual problem as well.

:)

Re: [PATCH 2/6] drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct adjusted_mode clock

[Alexander Stein]

Hi Marek,

Am Freitag, 21. Juni 2024, 16:54:51 CEST schrieb Marek Vasut:
> On 6/21/24 12:32 PM, Alexander Stein wrote:
> 
> Hi,
> 
> skipping the parts where I would simply write "OK" ...
> 
> >>>> As FVUEN is cleared at the next VSYNC event I suspect the DSI timings
> >>>> are (slightly) off, but unfortunately I don't have equipment to check
> >>>> DSI signal quality/timings.
> >>>
> >>> As long as the LCDIFv3 pixel clock are equal or slightly slower than
> >>> what the TC9595 PixelPLL generates, AND, DSIM serializer has enough
> >>> bandwidth on the DSI bus (i.e. set the bus to 1 GHz, the TC9595 DSI RX
> >>> cannot go any faster), you should have no issues on that end.
> > 
> > I'm using samsung,burst-clock-frequency = <1000000000> so this should be
> > okay. That is 1080p resolution.
> 
> Yes, correct.
> 
> >>> When in doubt, try and use i2ctransfer to read out register 0x300
> >>> repeatedly, that's DSI RX error counter register. See if the DSI error
> >>> count increments.
> > 
> > If the bridge is not working the registers look like this:
> > 300: c0800000
> > 464: 00000001
> > 
> > they are not changing and stay like that.
> > 
> > If the bridge is actually running they are like
> > 300: c08000d3
> > 464: 00000000
> > 
> > and are also not changing.
> 
> Uh ... that looks like the whole chip clock tree somehow locked up .
> 
> Thinking about this, I once did force the DSIM into 24 MHz mode (there 
> is PLL bypass setting, where the DSIM uses 24 MHz serializer clock 
> directly for the DSI HS clock) or something close, it was enough to 
> drive a low resolution panel. But the upside was, with a 200 MHz 5Gsps 
> scope set to AC-coupling and 10x probe, I could discern the traffic on 
> DSI data lane and decode it by hand. The nice thing is, you could 
> trigger on 1V2 LP mode, so you know where the packet starts. The 
> downside is, if you have multiple data lanes, the packet is spread 
> across them.
> 
> You could also tweak tc_edp_atomic_check()/tc_edp_mode_valid() and force 
> only low(er) resolution modes of your DP panel right from the start, so 
> you wouldn't need that much DSI bandwidth. Maybe you could reach some 
> mode where your equipment is enough to analyze the traffic by hand ?

I think I got it running now. Apparently there were different, independent
problems which you addressed by your series. Unfortunately the patch
'tc358767: Disable MIPI_DSI_CLOCK_NON_CONTINUOUS' introduced a new problem
(at least for me). For the record I'm running the following patch stack based
on next-20240621:
* Add linux-next specific files for 20240621
* arm64: dts: imx8mp: Add TC9595 DSI-DP bridge on TQMa8MPxL/MBa8MPxL
* drm: bridge: samsung-dsim: Initialize bridge on attach
* drm/bridge: tc358767: Reset chip again on attach
* drm: lcdif: Use adjusted_mode .clock instead of .crtc_clock
* drm/bridge: tc358767: Split tc_pxl_pll_en() into parameter calculation and enablement
* drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct adjusted_mode clock
* drm/bridge: tc358767: Drop line_pixel_subtract
* drm/bridge: tc358767: Set LSCLK divider for SYSCLK to 1
* Revert "drm/bridge: tc358767: Set default CLRSIPO count"

All of them are needed, reverting/dropping a single one results in a
non-functioning bridge.

Best regards,
Alexander
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH 2/6] drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct adjusted_mode clock

[Marek Vasut]

On 6/24/24 11:26 AM, Alexander Stein wrote:
> Hi Marek,

Hi,

> Am Freitag, 21. Juni 2024, 16:54:51 CEST schrieb Marek Vasut:
>> On 6/21/24 12:32 PM, Alexander Stein wrote:
>>
>> Hi,
>>
>> skipping the parts where I would simply write "OK" ...
>>
>>>>>> As FVUEN is cleared at the next VSYNC event I suspect the DSI timings
>>>>>> are (slightly) off, but unfortunately I don't have equipment to check
>>>>>> DSI signal quality/timings.
>>>>>
>>>>> As long as the LCDIFv3 pixel clock are equal or slightly slower than
>>>>> what the TC9595 PixelPLL generates, AND, DSIM serializer has enough
>>>>> bandwidth on the DSI bus (i.e. set the bus to 1 GHz, the TC9595 DSI RX
>>>>> cannot go any faster), you should have no issues on that end.
>>>
>>> I'm using samsung,burst-clock-frequency = <1000000000> so this should be
>>> okay. That is 1080p resolution.
>>
>> Yes, correct.
>>
>>>>> When in doubt, try and use i2ctransfer to read out register 0x300
>>>>> repeatedly, that's DSI RX error counter register. See if the DSI error
>>>>> count increments.
>>>
>>> If the bridge is not working the registers look like this:
>>> 300: c0800000
>>> 464: 00000001
>>>
>>> they are not changing and stay like that.
>>>
>>> If the bridge is actually running they are like
>>> 300: c08000d3
>>> 464: 00000000
>>>
>>> and are also not changing.
>>
>> Uh ... that looks like the whole chip clock tree somehow locked up .
>>
>> Thinking about this, I once did force the DSIM into 24 MHz mode (there
>> is PLL bypass setting, where the DSIM uses 24 MHz serializer clock
>> directly for the DSI HS clock) or something close, it was enough to
>> drive a low resolution panel. But the upside was, with a 200 MHz 5Gsps
>> scope set to AC-coupling and 10x probe, I could discern the traffic on
>> DSI data lane and decode it by hand. The nice thing is, you could
>> trigger on 1V2 LP mode, so you know where the packet starts. The
>> downside is, if you have multiple data lanes, the packet is spread
>> across them.
>>
>> You could also tweak tc_edp_atomic_check()/tc_edp_mode_valid() and force
>> only low(er) resolution modes of your DP panel right from the start, so
>> you wouldn't need that much DSI bandwidth. Maybe you could reach some
>> mode where your equipment is enough to analyze the traffic by hand ?
> 
> I think I got it running now. Apparently there were different, independent
> problems which you addressed by your series.

Oh, glad I could help.

> Unfortunately the patch
> 'tc358767: Disable MIPI_DSI_CLOCK_NON_CONTINUOUS' introduced a new problem
> (at least for me). For the record I'm running the following patch stack based
> on next-20240621:

Thanks for tracking it down. I can drop that one 
MIPI_DSI_CLOCK_NON_CONTINUOUS patch from the series and do a V4. Would 
that work for you ? At least there would be some improvement to the 
driver and I can analyze the MIPI_DSI_CLOCK_NON_CONTINUOUS issue in 
detail separately.

> * Add linux-next specific files for 20240621
> * arm64: dts: imx8mp: Add TC9595 DSI-DP bridge on TQMa8MPxL/MBa8MPxL
> * drm: bridge: samsung-dsim: Initialize bridge on attach
> * drm/bridge: tc358767: Reset chip again on attach
> * drm: lcdif: Use adjusted_mode .clock instead of .crtc_clock
> * drm/bridge: tc358767: Split tc_pxl_pll_en() into parameter calculation and enablement
> * drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct adjusted_mode clock
> * drm/bridge: tc358767: Drop line_pixel_subtract
> * drm/bridge: tc358767: Set LSCLK divider for SYSCLK to 1
> * Revert "drm/bridge: tc358767: Set default CLRSIPO count"
> 
> All of them are needed, reverting/dropping a single one results in a
> non-functioning bridge.

Thank you for testing !

Re: [PATCH 2/6] drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct adjusted_mode clock

[Alexander Stein]

Hi Marek,

Am Dienstag, 25. Juni 2024, 02:33:53 CEST schrieb Marek Vasut:
> On 6/24/24 11:26 AM, Alexander Stein wrote:
> > Hi Marek,
> 
> Hi,
> 
> > Am Freitag, 21. Juni 2024, 16:54:51 CEST schrieb Marek Vasut:
> >> On 6/21/24 12:32 PM, Alexander Stein wrote:
> >>
> >> Hi,
> >>
> >> skipping the parts where I would simply write "OK" ...
> >>
> >>>>>> As FVUEN is cleared at the next VSYNC event I suspect the DSI timings
> >>>>>> are (slightly) off, but unfortunately I don't have equipment to check
> >>>>>> DSI signal quality/timings.
> >>>>>
> >>>>> As long as the LCDIFv3 pixel clock are equal or slightly slower than
> >>>>> what the TC9595 PixelPLL generates, AND, DSIM serializer has enough
> >>>>> bandwidth on the DSI bus (i.e. set the bus to 1 GHz, the TC9595 DSI RX
> >>>>> cannot go any faster), you should have no issues on that end.
> >>>
> >>> I'm using samsung,burst-clock-frequency = <1000000000> so this should be
> >>> okay. That is 1080p resolution.
> >>
> >> Yes, correct.
> >>
> >>>>> When in doubt, try and use i2ctransfer to read out register 0x300
> >>>>> repeatedly, that's DSI RX error counter register. See if the DSI error
> >>>>> count increments.
> >>>
> >>> If the bridge is not working the registers look like this:
> >>> 300: c0800000
> >>> 464: 00000001
> >>>
> >>> they are not changing and stay like that.
> >>>
> >>> If the bridge is actually running they are like
> >>> 300: c08000d3
> >>> 464: 00000000
> >>>
> >>> and are also not changing.
> >>
> >> Uh ... that looks like the whole chip clock tree somehow locked up .
> >>
> >> Thinking about this, I once did force the DSIM into 24 MHz mode (there
> >> is PLL bypass setting, where the DSIM uses 24 MHz serializer clock
> >> directly for the DSI HS clock) or something close, it was enough to
> >> drive a low resolution panel. But the upside was, with a 200 MHz 5Gsps
> >> scope set to AC-coupling and 10x probe, I could discern the traffic on
> >> DSI data lane and decode it by hand. The nice thing is, you could
> >> trigger on 1V2 LP mode, so you know where the packet starts. The
> >> downside is, if you have multiple data lanes, the packet is spread
> >> across them.
> >>
> >> You could also tweak tc_edp_atomic_check()/tc_edp_mode_valid() and force
> >> only low(er) resolution modes of your DP panel right from the start, so
> >> you wouldn't need that much DSI bandwidth. Maybe you could reach some
> >> mode where your equipment is enough to analyze the traffic by hand ?
> > 
> > I think I got it running now. Apparently there were different, independent
> > problems which you addressed by your series.
> 
> Oh, glad I could help.
> 
> > Unfortunately the patch
> > 'tc358767: Disable MIPI_DSI_CLOCK_NON_CONTINUOUS' introduced a new problem
> > (at least for me). For the record I'm running the following patch stack based
> > on next-20240621:
> 
> Thanks for tracking it down. I can drop that one 
> MIPI_DSI_CLOCK_NON_CONTINUOUS patch from the series and do a V4. Would 
> that work for you ? At least there would be some improvement to the 
> driver and I can analyze the MIPI_DSI_CLOCK_NON_CONTINUOUS issue in 
> detail separately.

Sure, thanks to your patches this bridge does its job now.
Sure, now that I have a reference system I can easily try a V4 without
the MIPI_DSI_CLOCK_NON_CONTINUOUS patch.

Best regards,
Alexander

> > * Add linux-next specific files for 20240621
> > * arm64: dts: imx8mp: Add TC9595 DSI-DP bridge on TQMa8MPxL/MBa8MPxL
> > * drm: bridge: samsung-dsim: Initialize bridge on attach
> > * drm/bridge: tc358767: Reset chip again on attach
> > * drm: lcdif: Use adjusted_mode .clock instead of .crtc_clock
> > * drm/bridge: tc358767: Split tc_pxl_pll_en() into parameter calculation and enablement
> > * drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct adjusted_mode clock
> > * drm/bridge: tc358767: Drop line_pixel_subtract
> > * drm/bridge: tc358767: Set LSCLK divider for SYSCLK to 1
> > * Revert "drm/bridge: tc358767: Set default CLRSIPO count"
> > 
> > All of them are needed, reverting/dropping a single one results in a
> > non-functioning bridge.
> 
> Thank you for testing !
> 


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH 2/6] drm/bridge: tc358767: Use tc_pxl_pll_calc() to correct adjusted_mode clock

[Marek Vasut]

On 6/25/24 8:11 AM, Alexander Stein wrote:
> Hi Marek,
> 
> Am Dienstag, 25. Juni 2024, 02:33:53 CEST schrieb Marek Vasut:
>> On 6/24/24 11:26 AM, Alexander Stein wrote:
>>> Hi Marek,
>>
>> Hi,
>>
>>> Am Freitag, 21. Juni 2024, 16:54:51 CEST schrieb Marek Vasut:
>>>> On 6/21/24 12:32 PM, Alexander Stein wrote:
>>>>
>>>> Hi,
>>>>
>>>> skipping the parts where I would simply write "OK" ...
>>>>
>>>>>>>> As FVUEN is cleared at the next VSYNC event I suspect the DSI timings
>>>>>>>> are (slightly) off, but unfortunately I don't have equipment to check
>>>>>>>> DSI signal quality/timings.
>>>>>>>
>>>>>>> As long as the LCDIFv3 pixel clock are equal or slightly slower than
>>>>>>> what the TC9595 PixelPLL generates, AND, DSIM serializer has enough
>>>>>>> bandwidth on the DSI bus (i.e. set the bus to 1 GHz, the TC9595 DSI RX
>>>>>>> cannot go any faster), you should have no issues on that end.
>>>>>
>>>>> I'm using samsung,burst-clock-frequency = <1000000000> so this should be
>>>>> okay. That is 1080p resolution.
>>>>
>>>> Yes, correct.
>>>>
>>>>>>> When in doubt, try and use i2ctransfer to read out register 0x300
>>>>>>> repeatedly, that's DSI RX error counter register. See if the DSI error
>>>>>>> count increments.
>>>>>
>>>>> If the bridge is not working the registers look like this:
>>>>> 300: c0800000
>>>>> 464: 00000001
>>>>>
>>>>> they are not changing and stay like that.
>>>>>
>>>>> If the bridge is actually running they are like
>>>>> 300: c08000d3
>>>>> 464: 00000000
>>>>>
>>>>> and are also not changing.
>>>>
>>>> Uh ... that looks like the whole chip clock tree somehow locked up .
>>>>
>>>> Thinking about this, I once did force the DSIM into 24 MHz mode (there
>>>> is PLL bypass setting, where the DSIM uses 24 MHz serializer clock
>>>> directly for the DSI HS clock) or something close, it was enough to
>>>> drive a low resolution panel. But the upside was, with a 200 MHz 5Gsps
>>>> scope set to AC-coupling and 10x probe, I could discern the traffic on
>>>> DSI data lane and decode it by hand. The nice thing is, you could
>>>> trigger on 1V2 LP mode, so you know where the packet starts. The
>>>> downside is, if you have multiple data lanes, the packet is spread
>>>> across them.
>>>>
>>>> You could also tweak tc_edp_atomic_check()/tc_edp_mode_valid() and force
>>>> only low(er) resolution modes of your DP panel right from the start, so
>>>> you wouldn't need that much DSI bandwidth. Maybe you could reach some
>>>> mode where your equipment is enough to analyze the traffic by hand ?
>>>
>>> I think I got it running now. Apparently there were different, independent
>>> problems which you addressed by your series.
>>
>> Oh, glad I could help.
>>
>>> Unfortunately the patch
>>> 'tc358767: Disable MIPI_DSI_CLOCK_NON_CONTINUOUS' introduced a new problem
>>> (at least for me). For the record I'm running the following patch stack based
>>> on next-20240621:
>>
>> Thanks for tracking it down. I can drop that one
>> MIPI_DSI_CLOCK_NON_CONTINUOUS patch from the series and do a V4. Would
>> that work for you ? At least there would be some improvement to the
>> driver and I can analyze the MIPI_DSI_CLOCK_NON_CONTINUOUS issue in
>> detail separately.
> 
> Sure, thanks to your patches this bridge does its job now.
> Sure, now that I have a reference system I can easily try a V4 without
> the MIPI_DSI_CLOCK_NON_CONTINUOUS patch.

V4 is now out, thanks !