[PATCH v2 1/2] drm: bridge: samsung-dsim: Initialize bridge on attach

[PATCH v2 1/2] drm: bridge: samsung-dsim: Initialize bridge on attach

[Marek Vasut]

Initialize the bridge on attach already, to force lanes into LP11
state, since attach does trigger attach of downstream bridges which
may trigger (e)DP AUX channel mode read.

This fixes a corner case where DSIM with TC9595 attached to it fails
to operate the DP AUX channel, because the TC9595 enters some debug
mode when it is released from reset without lanes in LP11 mode. By
ensuring the DSIM lanes are in LP11, the TC9595 (tc358767.c driver)
can be reset in its attach callback called from DSIM attach callback,
and recovered out of the debug mode just before TC9595 performs first
AUX channel access later in its attach callback.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Adam Ford <aford173@gmail.com>
Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
Cc: Andrzej Hajda <andrzej.hajda@intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: David Airlie <airlied@gmail.com>
Cc: Frieder Schrempf <frieder.schrempf@kontron.de>
Cc: Inki Dae <inki.dae@samsung.com>
Cc: Jagan Teki <jagan@amarulasolutions.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: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Michael Walle <mwalle@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
---
V2: Handle case where mode is not set yet
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 32 ++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index e7e53a9e42afb..22d3bbd866d97 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -699,20 +699,24 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
 
 static int samsung_dsim_enable_clock(struct samsung_dsim *dsi)
 {
-	unsigned long hs_clk, byte_clk, esc_clk, pix_clk;
+	unsigned long hs_clk, byte_clk, esc_clk;
 	unsigned long esc_div;
 	u32 reg;
 	struct drm_display_mode *m = &dsi->mode;
 	int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
 
-	/* m->clock is in KHz */
-	pix_clk = m->clock * 1000;
-
-	/* Use burst_clk_rate if available, otherwise use the pix_clk */
+	/*
+	 * Use burst_clk_rate if available, otherwise use the mode clock
+	 * if mode is already set and available, otherwise fall back to
+	 * PLL input clock and operate in 1:1 lowest frequency mode until
+	 * a mode is set.
+	 */
 	if (dsi->burst_clk_rate)
 		hs_clk = samsung_dsim_set_pll(dsi, dsi->burst_clk_rate);
+	else if (m)	/* m->clock is in KHz */
+		hs_clk = samsung_dsim_set_pll(dsi, DIV_ROUND_UP(m->clock * 1000 * bpp, dsi->lanes));
 	else
-		hs_clk = samsung_dsim_set_pll(dsi, DIV_ROUND_UP(pix_clk * bpp, dsi->lanes));
+		hs_clk = dsi->pll_clk_rate;
 
 	if (!hs_clk) {
 		dev_err(dsi->dev, "failed to configure DSI PLL\n");
@@ -1643,9 +1647,21 @@ static int samsung_dsim_attach(struct drm_bridge *bridge,
 			       enum drm_bridge_attach_flags flags)
 {
 	struct samsung_dsim *dsi = bridge_to_dsi(bridge);
+	int ret;
 
-	return drm_bridge_attach(bridge->encoder, dsi->out_bridge, bridge,
-				 flags);
+	ret = pm_runtime_resume_and_get(dsi->dev);
+	if (ret < 0)
+		return ret;
+
+	ret = samsung_dsim_init(dsi);
+	if (ret < 0)
+		goto err;
+
+	ret = drm_bridge_attach(bridge->encoder, dsi->out_bridge, bridge,
+				flags);
+err:
+	pm_runtime_put_sync(dsi->dev);
+	return ret;
 }
 
 static const struct drm_bridge_funcs samsung_dsim_bridge_funcs = {
-- 
2.43.0

[PATCH v2 2/2] drm/bridge: tc358767: Reset chip again on attach

[Marek Vasut]

In case the chip is released from reset using the RESX signal while the
DSI lanes are in non-LP11 mode, the chip may enter some sort of debug
mode, where its internal clock run at 1/6th of expected clock rate. In
this mode, the AUX channel also operates at 1/6th of the 10 MHz mandated
by DP specification, which breaks DPCD communication.

There is no known software way of bringing the chip out of this state
once the chip enters it, except for toggling the RESX signal and
performing full reset.

The chip may enter this mode when the chip was released from reset in
probe(), because at that point the DSI lane mode is undefined.

When the .attach callback is called, the DSI link is surely in LP11 mode.
Toggle the RESX signal here and reconfigure the AUX channel. That way,
the AUX channel communication from this point on does surely run at
10 MHz as it should.

Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Adam Ford <aford173@gmail.com>
Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
Cc: Andrzej Hajda <andrzej.hajda@intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: David Airlie <airlied@gmail.com>
Cc: Frieder Schrempf <frieder.schrempf@kontron.de>
Cc: Inki Dae <inki.dae@samsung.com>
Cc: Jagan Teki <jagan@amarulasolutions.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: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Michael Walle <mwalle@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
---
V2: Add RB from Alexander
---
 drivers/gpu/drm/bridge/tc358767.c | 50 +++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 6bda26ca6b917..7d8797b3d8579 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1749,10 +1749,30 @@ static const struct drm_connector_funcs tc_connector_funcs = {
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
+static void tc_bridge_reset(struct tc_data *tc)
+{
+	if (!tc->reset_gpio)
+		return;
+
+	gpiod_set_value_cansleep(tc->reset_gpio, 0);
+	usleep_range(10000, 11000);
+	gpiod_set_value_cansleep(tc->reset_gpio, 1);
+	usleep_range(5000, 10000);
+}
+
 static int tc_dpi_bridge_attach(struct drm_bridge *bridge,
 				enum drm_bridge_attach_flags flags)
 {
 	struct tc_data *tc = bridge_to_tc(bridge);
+	int ret;
+
+	if (tc->reset_gpio) {
+		tc_bridge_reset(tc);
+
+		ret = tc_set_syspllparam(tc);
+		if (ret)
+			return ret;
+	}
 
 	if (!tc->panel_bridge)
 		return 0;
@@ -1769,6 +1789,36 @@ static int tc_edp_bridge_attach(struct drm_bridge *bridge,
 	struct drm_device *drm = bridge->dev;
 	int ret;
 
+	if (tc->reset_gpio) {
+		/*
+		 * In case the chip is released from reset using the RESX
+		 * signal while the DSI lanes are in non-LP11 mode, the chip
+		 * may enter some sort of debug mode, where its internal
+		 * clock run at 1/6th of expected clock rate. In this mode,
+		 * the AUX channel also operates at 1/6th of the 10 MHz
+		 * mandated by DP specification, which breaks DPCD
+		 * communication.
+		 *
+		 * There is no known software way of bringing the chip out of
+		 * this state once the chip enters it, except for toggling
+		 * the RESX signal and performing full reset.
+		 *
+		 * The chip may enter this mode when the chip was released
+		 * from reset in probe(), because at that point the DSI lane
+		 * mode is undefined.
+		 *
+		 * At this point, the DSI link is surely in LP11 mode. Toggle
+		 * the RESX signal here and reconfigure the AUX channel. That
+		 * way, the AUX channel communication from this point on does
+		 * surely run at 10 MHz as it should.
+		 */
+		tc_bridge_reset(tc);
+
+		ret = tc_aux_link_setup(tc);
+		if (ret)
+			return ret;
+	}
+
 	if (tc->panel_bridge) {
 		/* If a connector is required then this driver shall create it */
 		ret = drm_bridge_attach(tc->bridge.encoder, tc->panel_bridge,
-- 
2.43.0

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Initialize bridge on attach

[Alexander Stein]

Hi Marek,

Am Dienstag, 25. Juni 2024, 14:26:10 CEST schrieb Marek Vasut:
> Initialize the bridge on attach already, to force lanes into LP11
> state, since attach does trigger attach of downstream bridges which
> may trigger (e)DP AUX channel mode read.
> 
> This fixes a corner case where DSIM with TC9595 attached to it fails
> to operate the DP AUX channel, because the TC9595 enters some debug
> mode when it is released from reset without lanes in LP11 mode. By
> ensuring the DSIM lanes are in LP11, the TC9595 (tc358767.c driver)
> can be reset in its attach callback called from DSIM attach callback,
> and recovered out of the debug mode just before TC9595 performs first
> AUX channel access later in its attach callback.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Adam Ford <aford173@gmail.com>
> Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Frieder Schrempf <frieder.schrempf@kontron.de>
> Cc: Inki Dae <inki.dae@samsung.com>
> Cc: Jagan Teki <jagan@amarulasolutions.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: Marek Szyprowski <m.szyprowski@samsung.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Michael Walle <mwalle@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
> ---
> V2: Handle case where mode is not set yet
> ---
>  drivers/gpu/drm/bridge/samsung-dsim.c | 32 ++++++++++++++++++++-------
>  1 file changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index e7e53a9e42afb..22d3bbd866d97 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -699,20 +699,24 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
>  
>  static int samsung_dsim_enable_clock(struct samsung_dsim *dsi)
>  {
> -	unsigned long hs_clk, byte_clk, esc_clk, pix_clk;
> +	unsigned long hs_clk, byte_clk, esc_clk;
>  	unsigned long esc_div;
>  	u32 reg;
>  	struct drm_display_mode *m = &dsi->mode;
>  	int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
>  
> -	/* m->clock is in KHz */
> -	pix_clk = m->clock * 1000;
> -
> -	/* Use burst_clk_rate if available, otherwise use the pix_clk */
> +	/*
> +	 * Use burst_clk_rate if available, otherwise use the mode clock
> +	 * if mode is already set and available, otherwise fall back to
> +	 * PLL input clock and operate in 1:1 lowest frequency mode until
> +	 * a mode is set.
> +	 */
>  	if (dsi->burst_clk_rate)
>  		hs_clk = samsung_dsim_set_pll(dsi, dsi->burst_clk_rate);
> +	else if (m)	/* m->clock is in KHz */
> +		hs_clk = samsung_dsim_set_pll(dsi, DIV_ROUND_UP(m->clock * 1000 * bpp, dsi->lanes));
>  	else
> -		hs_clk = samsung_dsim_set_pll(dsi, DIV_ROUND_UP(pix_clk * bpp, dsi->lanes));
> +		hs_clk = dsi->pll_clk_rate;
>  

I can't reproduce that mentioned corner case and presumably this problem
does not exist otherwise. If samsung,burst-clock-frequency is unset I get
a sluggish image on the monitor.

It seems the calculation is using a adjusted clock frequency:
samsung-dsim 32e60000.dsi: Using pixel clock for HS clock frequency
samsung-dsim 32e60000.dsi: [drm:samsung_dsim_host_attach [samsung_dsim]] Attached tc358767 device (lanes:4 bpp:24 mode-flags:0xc03)
samsung-dsim 32e60000.dsi: PLL ref clock freq 24000000
samsung-dsim 32e60000.dsi: failed to find PLL PMS for requested frequency
samsung-dsim 32e60000.dsi: failed to configure DSI PLL
samsung-dsim 32e60000.dsi: PLL ref clock freq 24000000
samsung-dsim 32e60000.dsi: PLL freq 883636363, (p 11, m 810, s 1)
samsung-dsim 32e60000.dsi: hs_clk = 883636363, byte_clk = 110454545, esc_clk = 9204545
samsung-dsim 32e60000.dsi: calculated hfp: 60, hbp: 105, hsa: 27
samsung-dsim 32e60000.dsi: LCD size = 1920x1080

891 MHz is the nominal one for 148.5 MHz pixelclock. But even setting
samsung,burst-clock-frequency to 891 MHz results in a sluggish image.
Maybe this usecase is nothing I need to consider while using DSI-DP
with EDID timings available.

As the burst clock needs to provide more bandwidth than actually needed,
I consider this a different usecase not providing
samsung,burst-clock-frequency for DSI->DP usage.

But the initialization upon attach is working intended, so:
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>

>  	if (!hs_clk) {
>  		dev_err(dsi->dev, "failed to configure DSI PLL\n");
> @@ -1643,9 +1647,21 @@ static int samsung_dsim_attach(struct drm_bridge *bridge,
>  			       enum drm_bridge_attach_flags flags)
>  {
>  	struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> +	int ret;
>  
> -	return drm_bridge_attach(bridge->encoder, dsi->out_bridge, bridge,
> -				 flags);
> +	ret = pm_runtime_resume_and_get(dsi->dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = samsung_dsim_init(dsi);
> +	if (ret < 0)
> +		goto err;
> +
> +	ret = drm_bridge_attach(bridge->encoder, dsi->out_bridge, bridge,
> +				flags);
> +err:
> +	pm_runtime_put_sync(dsi->dev);
> +	return ret;
>  }
>  
>  static const struct drm_bridge_funcs samsung_dsim_bridge_funcs = {
> 


-- 
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 v2 1/2] drm: bridge: samsung-dsim: Initialize bridge on attach

[Marek Vasut]

On 6/25/24 4:37 PM, Alexander Stein wrote:
> Hi Marek,

Hi,

> Am Dienstag, 25. Juni 2024, 14:26:10 CEST schrieb Marek Vasut:
>> Initialize the bridge on attach already, to force lanes into LP11
>> state, since attach does trigger attach of downstream bridges which
>> may trigger (e)DP AUX channel mode read.
>>
>> This fixes a corner case where DSIM with TC9595 attached to it fails
>> to operate the DP AUX channel, because the TC9595 enters some debug
>> mode when it is released from reset without lanes in LP11 mode. By
>> ensuring the DSIM lanes are in LP11, the TC9595 (tc358767.c driver)
>> can be reset in its attach callback called from DSIM attach callback,
>> and recovered out of the debug mode just before TC9595 performs first
>> AUX channel access later in its attach callback.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> ---
>> Cc: Adam Ford <aford173@gmail.com>
>> Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
>> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Cc: David Airlie <airlied@gmail.com>
>> Cc: Frieder Schrempf <frieder.schrempf@kontron.de>
>> Cc: Inki Dae <inki.dae@samsung.com>
>> Cc: Jagan Teki <jagan@amarulasolutions.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: Marek Szyprowski <m.szyprowski@samsung.com>
>> Cc: Maxime Ripard <mripard@kernel.org>
>> Cc: Michael Walle <mwalle@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
>> ---
>> V2: Handle case where mode is not set yet
>> ---
>>   drivers/gpu/drm/bridge/samsung-dsim.c | 32 ++++++++++++++++++++-------
>>   1 file changed, 24 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
>> index e7e53a9e42afb..22d3bbd866d97 100644
>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>> @@ -699,20 +699,24 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
>>   
>>   static int samsung_dsim_enable_clock(struct samsung_dsim *dsi)
>>   {
>> -	unsigned long hs_clk, byte_clk, esc_clk, pix_clk;
>> +	unsigned long hs_clk, byte_clk, esc_clk;
>>   	unsigned long esc_div;
>>   	u32 reg;
>>   	struct drm_display_mode *m = &dsi->mode;
>>   	int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
>>   
>> -	/* m->clock is in KHz */
>> -	pix_clk = m->clock * 1000;
>> -
>> -	/* Use burst_clk_rate if available, otherwise use the pix_clk */
>> +	/*
>> +	 * Use burst_clk_rate if available, otherwise use the mode clock
>> +	 * if mode is already set and available, otherwise fall back to
>> +	 * PLL input clock and operate in 1:1 lowest frequency mode until
>> +	 * a mode is set.
>> +	 */
>>   	if (dsi->burst_clk_rate)
>>   		hs_clk = samsung_dsim_set_pll(dsi, dsi->burst_clk_rate);
>> +	else if (m)	/* m->clock is in KHz */
>> +		hs_clk = samsung_dsim_set_pll(dsi, DIV_ROUND_UP(m->clock * 1000 * bpp, dsi->lanes));
>>   	else
>> -		hs_clk = samsung_dsim_set_pll(dsi, DIV_ROUND_UP(pix_clk * bpp, dsi->lanes));
>> +		hs_clk = dsi->pll_clk_rate;
>>   
> 
> I can't reproduce that mentioned corner case and presumably this problem
> does not exist otherwise. If samsung,burst-clock-frequency is unset I get
> a sluggish image on the monitor.
> 
> It seems the calculation is using a adjusted clock frequency:
> samsung-dsim 32e60000.dsi: Using pixel clock for HS clock frequency
> samsung-dsim 32e60000.dsi: [drm:samsung_dsim_host_attach [samsung_dsim]] Attached tc358767 device (lanes:4 bpp:24 mode-flags:0xc03)
> samsung-dsim 32e60000.dsi: PLL ref clock freq 24000000
> samsung-dsim 32e60000.dsi: failed to find PLL PMS for requested frequency
> samsung-dsim 32e60000.dsi: failed to configure DSI PLL
> samsung-dsim 32e60000.dsi: PLL ref clock freq 24000000
> samsung-dsim 32e60000.dsi: PLL freq 883636363, (p 11, m 810, s 1)
> samsung-dsim 32e60000.dsi: hs_clk = 883636363, byte_clk = 110454545, esc_clk = 9204545
> samsung-dsim 32e60000.dsi: calculated hfp: 60, hbp: 105, hsa: 27
> samsung-dsim 32e60000.dsi: LCD size = 1920x1080
> 
> 891 MHz is the nominal one for 148.5 MHz pixelclock. But even setting
> samsung,burst-clock-frequency to 891 MHz results in a sluggish image.
> Maybe this usecase is nothing I need to consider while using DSI-DP
> with EDID timings available.
> 
> As the burst clock needs to provide more bandwidth than actually needed,
> I consider this a different usecase not providing
> samsung,burst-clock-frequency for DSI->DP usage.
> 
> But the initialization upon attach is working intended, so:
> Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>

Thank you for testing and keeping up with this. I will wait for more 
feedback if there is any (Frieder? Lucas? Michael?). If there are no 
objections, then I can merge it in a week or two ?

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Initialize bridge on attach

[Michael Walle]

[-- Attachment #1: Type: text/plain, Size: 362 bytes --]

On Wed Jun 26, 2024 at 5:21 AM CEST, Marek Vasut wrote:
> Thank you for testing and keeping up with this. I will wait for more 
> feedback if there is any (Frieder? Lucas? Michael?). If there are no 
> objections, then I can merge it in a week or two ?

I'll try to use your approach on the tc358775. Hopefully, I'll find
some time this week.

-michael

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Initialize bridge on attach

[Marek Vasut]

On 6/26/24 10:02 AM, Michael Walle wrote:
> On Wed Jun 26, 2024 at 5:21 AM CEST, Marek Vasut wrote:
>> Thank you for testing and keeping up with this. I will wait for more
>> feedback if there is any (Frieder? Lucas? Michael?). If there are no
>> objections, then I can merge it in a week or two ?
> 
> I'll try to use your approach on the tc358775. Hopefully, I'll find
> some time this week.

So ... I wonder ... shall I apply these patches or not ?

I'll wait about a week or two before applying them, to get some input.

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Initialize bridge on attach

[Marek Szyprowski]

On 11.07.2024 17:38, Marek Vasut wrote:
> On 6/26/24 10:02 AM, Michael Walle wrote:
>> On Wed Jun 26, 2024 at 5:21 AM CEST, Marek Vasut wrote:
>>> Thank you for testing and keeping up with this. I will wait for more
>>> feedback if there is any (Frieder? Lucas? Michael?). If there are no
>>> objections, then I can merge it in a week or two ?
>>
>> I'll try to use your approach on the tc358775. Hopefully, I'll find
>> some time this week.
>
> So ... I wonder ... shall I apply these patches or not ?
>
> I'll wait about a week or two before applying them, to get some input.
>
I've pointed that they break current users of Samsung DSIM: Exynos-DSI 
and Samsung s6e3ha2/s6e3hf2 panels, but unfortunately I'm not able to 
provide datasheet nor any other documentation. Due to other tasks and 
holidays I'm not able to debug it further too, at least till the end of 
August. Maybe we could keep old behavior for "exynos-dsi" compatible device?

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Initialize bridge on attach

[Marek Vasut]

On 7/11/24 5:57 PM, Marek Szyprowski wrote:
> On 11.07.2024 17:38, Marek Vasut wrote:
>> On 6/26/24 10:02 AM, Michael Walle wrote:
>>> On Wed Jun 26, 2024 at 5:21 AM CEST, Marek Vasut wrote:
>>>> Thank you for testing and keeping up with this. I will wait for more
>>>> feedback if there is any (Frieder? Lucas? Michael?). If there are no
>>>> objections, then I can merge it in a week or two ?
>>>
>>> I'll try to use your approach on the tc358775. Hopefully, I'll find
>>> some time this week.
>>
>> So ... I wonder ... shall I apply these patches or not ?
>>
>> I'll wait about a week or two before applying them, to get some input.
>>
> I've pointed that they break current users of Samsung DSIM: Exynos-DSI
> and Samsung s6e3ha2/s6e3hf2 panels, but unfortunately I'm not able to
> provide datasheet nor any other documentation. Due to other tasks and
> holidays I'm not able to debug it further too, at least till the end of
> August. Maybe we could keep old behavior for "exynos-dsi" compatible device?

Nope, let's not pile up workarounds. It would be good to figure out why 
does this display misbehave when the data lanes are in LP11 on start up. 
It seems most sinks do require data lanes in LP11 on start up, so what 
is special about this panel ? Do you have access to the datasheet and 
can you check once you're back ?

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Initialize bridge on attach

[Michael Walle]

[-- Attachment #1: Type: text/plain, Size: 861 bytes --]

Hi Marek,

> >> Thank you for testing and keeping up with this. I will wait for more
> >> feedback if there is any (Frieder? Lucas? Michael?). If there are no
> >> objections, then I can merge it in a week or two ?
> > 
> > I'll try to use your approach on the tc358775. Hopefully, I'll find
> > some time this week.
>
> So ... I wonder ... shall I apply these patches or not ?

As mentioned on IRC, I tried it to port it for the mediatek DSI
host, but I gave up and got doubts that this is the way to go. I
think this is too invasive (in a sense that it changes behavior) and
not that easy to implement on other drivers.

Given that this requirement is far more common across DSI bridges,
I'd favor a more general solution which isn't a workaround.

-michael

>
> I'll wait about a week or two before applying them, to get some input.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Initialize bridge on attach

[Marek Vasut]

On 7/12/24 9:16 AM, Michael Walle wrote:
> Hi Marek,

Hi,

>>>> Thank you for testing and keeping up with this. I will wait for more
>>>> feedback if there is any (Frieder? Lucas? Michael?). If there are no
>>>> objections, then I can merge it in a week or two ?
>>>
>>> I'll try to use your approach on the tc358775. Hopefully, I'll find
>>> some time this week.
>>
>> So ... I wonder ... shall I apply these patches or not ?
> 
> As mentioned on IRC, I tried it to port it for the mediatek DSI
> host, but I gave up and got doubts that this is the way to go. I
> think this is too invasive (in a sense that it changes behavior)

I would argue it makes the behavior well defined. If that breaks some 
drivers that depended on the undefined behavior before, those should be 
fixed too.

> and not that easy to implement on other drivers.

How so ? At least the DSIM and STM32 DW DSI host can switch lanes to 
LP11 state. Is the mediatek host not capable of that ?

> Given that this requirement is far more common across DSI bridges,
> I'd favor a more general solution which isn't a workaround.

I think we only had a look at the TI DSI83 / ICN6211 / Toshiba TC358767 
bridges, but we did not look at many panels, did we ? Do panels require 
lanes in non-LP11 state on start up ?

Was there any progress on the generic LP11 solution, I think you did 
mention something was in progress ? How would that even look like ?

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Initialize bridge on attach

[Michael Walle]

> >>>> Thank you for testing and keeping up with this. I will wait for more
> >>>> feedback if there is any (Frieder? Lucas? Michael?). If there are no
> >>>> objections, then I can merge it in a week or two ?
> >>>
> >>> I'll try to use your approach on the tc358775. Hopefully, I'll find
> >>> some time this week.
> >>
> >> So ... I wonder ... shall I apply these patches or not ?
> > 
> > As mentioned on IRC, I tried it to port it for the mediatek DSI
> > host, but I gave up and got doubts that this is the way to go. I
> > think this is too invasive (in a sense that it changes behavior)
>
> I would argue it makes the behavior well defined. If that breaks some 
> drivers that depended on the undefined behavior before, those should be 
> fixed too.

Then this behavior should be documented (and accepted) in the
corresponding section:
https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation

This will help DSI host driver developers and we can point all the
host DSI driver maintainers to that document along with the proper
implementation :)

> > and not that easy to implement on other drivers.
>
> How so ? At least the DSIM and STM32 DW DSI host can switch lanes to 
> LP11 state. Is the mediatek host not capable of that ?

The controller is certainly capable to do that. But the changes
seems pretty invasive to me and I fear some fallout. Although it's
basically just one line for the DSIM, you seem to be moving the init
of the DSIM to an earlier point(?). I'm no expert with all the DRM
stuff, so I might be wrong here.

> > Given that this requirement is far more common across DSI bridges,
> > I'd favor a more general solution which isn't a workaround.
>
> I think we only had a look at the TI DSI83 / ICN6211 / Toshiba TC358767 
> bridges, but we did not look at many panels, did we ? Do panels require 
> lanes in non-LP11 state on start up ?

I'm not talking about panels, just bridges. It's not just one bridge
with a weird behavior but most bridges.

> Was there any progress on the generic LP11 solution, I think you did 
> mention something was in progress ? How would that even look like ?

I had a new callback implemented:
https://lore.kernel.org/r/20240506-tc358775-fix-powerup-v1-1-545dcf00b8dd@kernel.org/

Not sure if that's any better though.

-michael

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Initialize bridge on attach

[Marek Vasut]

On 7/16/24 9:50 PM, Michael Walle wrote:
>>>>>> Thank you for testing and keeping up with this. I will wait for more
>>>>>> feedback if there is any (Frieder? Lucas? Michael?). If there are no
>>>>>> objections, then I can merge it in a week or two ?
>>>>>
>>>>> I'll try to use your approach on the tc358775. Hopefully, I'll find
>>>>> some time this week.
>>>>
>>>> So ... I wonder ... shall I apply these patches or not ?
>>>
>>> As mentioned on IRC, I tried it to port it for the mediatek DSI
>>> host, but I gave up and got doubts that this is the way to go. I
>>> think this is too invasive (in a sense that it changes behavior)
>>
>> I would argue it makes the behavior well defined. If that breaks some
>> drivers that depended on the undefined behavior before, those should be
>> fixed too.
> 
> Then this behavior should be documented (and accepted) in the
> corresponding section:
> https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation

I think so too.

> This will help DSI host driver developers and we can point all the
> host DSI driver maintainers to that document along with the proper
> implementation :)
> 
>>> and not that easy to implement on other drivers.
>>
>> How so ? At least the DSIM and STM32 DW DSI host can switch lanes to
>> LP11 state. Is the mediatek host not capable of that ?
> 
> The controller is certainly capable to do that. But the changes
> seems pretty invasive to me and I fear some fallout. Although it's
> basically just one line for the DSIM, you seem to be moving the init
> of the DSIM to an earlier point(?). I'm no expert with all the DRM
> stuff, so I might be wrong here.

I am moving some of the initialization to an earlier point, but only 
enough of it to configure the lanes to LP11 state before the next 
bridge(s) start to (pre)enable themselves. And the DSIM controller is 
RPM suspended again after the lanes are in LP11.

>>> Given that this requirement is far more common across DSI bridges,
>>> I'd favor a more general solution which isn't a workaround.
>>
>> I think we only had a look at the TI DSI83 / ICN6211 / Toshiba TC358767
>> bridges, but we did not look at many panels, did we ? Do panels require
>> lanes in non-LP11 state on start up ?
> 
> I'm not talking about panels, just bridges. It's not just one bridge
> with a weird behavior but most bridges.

What do you refer to by "weird behavior" ? It seems the DSI bridges we 
looked at all required data lanes in LP11 state on start up one way or 
the other, didn't they ?

>> Was there any progress on the generic LP11 solution, I think you did
>> mention something was in progress ? How would that even look like ?
> 
> I had a new callback implemented:
> https://lore.kernel.org/r/20240506-tc358775-fix-powerup-v1-1-545dcf00b8dd@kernel.org/
> 
> Not sure if that's any better though.

Wouldn't that callback be called by various controllers at various 
stages of initialization , without consistency on when that callback 
will be called ? That would be my concern.

At least here, the expectation is that the controller would put data 
lanes into LP11 before the next bridge can even pre_enable itself , 
which is not perfect though, because if a bridge needs DSI clock to 
probe() itself and then data lanes in LP11 to probe itself, that is a 
really bad situation (and the TC358767 is capable of being used that 
way, although this is currently not supported by the kernel driver and 
there is no real interest in supporting it).

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Initialize bridge on attach

[Alexander Stein]

Hi,

with more and more patches for TC9595 support got meged into linux-next,
only a few remain on my patch stack.

This is one of them and is necessary for DP support:
Tested-by: Alexander Stein <alexander.stein@ew.tq-group.com>

Am Dienstag, 25. Juni 2024, 14:26:10 CEST schrieb Marek Vasut:
> Initialize the bridge on attach already, to force lanes into LP11
> state, since attach does trigger attach of downstream bridges which
> may trigger (e)DP AUX channel mode read.
> 
> This fixes a corner case where DSIM with TC9595 attached to it fails
> to operate the DP AUX channel, because the TC9595 enters some debug
> mode when it is released from reset without lanes in LP11 mode. By
> ensuring the DSIM lanes are in LP11, the TC9595 (tc358767.c driver)
> can be reset in its attach callback called from DSIM attach callback,
> and recovered out of the debug mode just before TC9595 performs first
> AUX channel access later in its attach callback.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Adam Ford <aford173@gmail.com>
> Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Frieder Schrempf <frieder.schrempf@kontron.de>
> Cc: Inki Dae <inki.dae@samsung.com>
> Cc: Jagan Teki <jagan@amarulasolutions.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: Marek Szyprowski <m.szyprowski@samsung.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Michael Walle <mwalle@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
> ---
> V2: Handle case where mode is not set yet
> ---
>  drivers/gpu/drm/bridge/samsung-dsim.c | 32 ++++++++++++++++++++-------
>  1 file changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index e7e53a9e42afb..22d3bbd866d97 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -699,20 +699,24 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
>  
>  static int samsung_dsim_enable_clock(struct samsung_dsim *dsi)
>  {
> -	unsigned long hs_clk, byte_clk, esc_clk, pix_clk;
> +	unsigned long hs_clk, byte_clk, esc_clk;
>  	unsigned long esc_div;
>  	u32 reg;
>  	struct drm_display_mode *m = &dsi->mode;
>  	int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
>  
> -	/* m->clock is in KHz */
> -	pix_clk = m->clock * 1000;
> -
> -	/* Use burst_clk_rate if available, otherwise use the pix_clk */
> +	/*
> +	 * Use burst_clk_rate if available, otherwise use the mode clock
> +	 * if mode is already set and available, otherwise fall back to
> +	 * PLL input clock and operate in 1:1 lowest frequency mode until
> +	 * a mode is set.
> +	 */
>  	if (dsi->burst_clk_rate)
>  		hs_clk = samsung_dsim_set_pll(dsi, dsi->burst_clk_rate);
> +	else if (m)	/* m->clock is in KHz */
> +		hs_clk = samsung_dsim_set_pll(dsi, DIV_ROUND_UP(m->clock * 1000 * bpp, dsi->lanes));
>  	else
> -		hs_clk = samsung_dsim_set_pll(dsi, DIV_ROUND_UP(pix_clk * bpp, dsi->lanes));
> +		hs_clk = dsi->pll_clk_rate;
>  
>  	if (!hs_clk) {
>  		dev_err(dsi->dev, "failed to configure DSI PLL\n");
> @@ -1643,9 +1647,21 @@ static int samsung_dsim_attach(struct drm_bridge *bridge,
>  			       enum drm_bridge_attach_flags flags)
>  {
>  	struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> +	int ret;
>  
> -	return drm_bridge_attach(bridge->encoder, dsi->out_bridge, bridge,
> -				 flags);
> +	ret = pm_runtime_resume_and_get(dsi->dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = samsung_dsim_init(dsi);
> +	if (ret < 0)
> +		goto err;
> +
> +	ret = drm_bridge_attach(bridge->encoder, dsi->out_bridge, bridge,
> +				flags);
> +err:
> +	pm_runtime_put_sync(dsi->dev);
> +	return ret;
>  }
>  
>  static const struct drm_bridge_funcs samsung_dsim_bridge_funcs = {
> 


-- 
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 v2 1/2] drm: bridge: samsung-dsim: Initialize bridge on attach

[Michael Walle]

[-- Attachment #1: Type: text/plain, Size: 1103 bytes --]

Hi,

> with more and more patches for TC9595 support got meged into linux-next,
> only a few remain on my patch stack.
>
> This is one of them and is necessary for DP support:
> Tested-by: Alexander Stein <alexander.stein@ew.tq-group.com>

As mentioned in [1] I expect a new version of this series with a
proper documentation update that will then (hopefully) be acked by
the maintainers.

Apart from that, I'm still not convinced that the .attach() callback
is the correct place to put the lanes into LP-11 mode. E.g. the
MediaTek DSI host needs to already know the HS clock rate before
configuring the D-PHY. Something which isn't available at the time
.attach() is called. Marek mentioned, that one should start with
lowest possible clock rate and later reconfigure it to the correct
rate. I'm not sure this is possible without taking the clock lanes
out of LP-11 mode again (i.e. disabling the PHY altogether to change
its PLL). But doing so will violate the bridge requirements again.

-michael

[1] https://lore.kernel.org/dri-devel/D2R83VFPUWE3.3MBX3LQOCDFWA@kernel.org/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]