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

[PATCH 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
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index 95fedc68b0ae5..56093fc3d62cc 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -1622,9 +1622,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;
+
+	ret = pm_runtime_resume_and_get(dsi->dev);
+	if (ret < 0)
+		return ret;
 
-	return drm_bridge_attach(bridge->encoder, dsi->out_bridge, bridge,
-				 flags);
+	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 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.

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
---
 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 fe2b93546eaef..9b01dc885973c 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: [2/2] drm/bridge: tc358767: Reset chip again on attach

[Alexander Stein]

Hi,

Am Montag, 13. Mai 2024, 04:16:28 CEST schrieb 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.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>

This does the trick on my hardware as well.
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
> 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
> ---
>  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 fe2b93546eaef..9b01dc885973c 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,
> 


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

[Marek Szyprowski]

On 13.05.2024 04:16, Marek Vasut wrote:
> 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>

This patch breaks driver operation on Samsung TM2e board with S6E3HF2 
DSI panel. The initialization procedure is very fragile and it looks 
that the changes must be done very carefully. We discussed this many 
times when converting this driver from Exynos DSI to generic Samsung DSI 
used on IMX and other SoCs.

> ---
> 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
> ---
>   drivers/gpu/drm/bridge/samsung-dsim.c | 16 ++++++++++++++--
>   1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 95fedc68b0ae5..56093fc3d62cc 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -1622,9 +1622,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;
> +
> +	ret = pm_runtime_resume_and_get(dsi->dev);
> +	if (ret < 0)
> +		return ret;
>   
> -	return drm_bridge_attach(bridge->encoder, dsi->out_bridge, bridge,
> -				 flags);
> +	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 = {

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

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

[Marek Vasut]

On 5/13/24 9:57 AM, Marek Szyprowski wrote:
> 
> On 13.05.2024 04:16, Marek Vasut wrote:
>> 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>
> 
> This patch breaks driver operation on Samsung TM2e board with S6E3HF2
> DSI panel. The initialization procedure is very fragile and it looks
> that the changes must be done very carefully. We discussed this many
> times when converting this driver from Exynos DSI to generic Samsung DSI
> used on IMX and other SoCs.

Clearly we need to find a way to support both use cases .

What options do we have ?

Does the panel require DSI lanes in some specific state when it is 
released from reset ?

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

[Alexander Stein]

Hi Marek,

thanks for the patch.

Am Montag, 13. Mai 2024, 04:16:27 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
> ---
>  drivers/gpu/drm/bridge/samsung-dsim.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 95fedc68b0ae5..56093fc3d62cc 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -1622,9 +1622,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;
> +
> +	ret = pm_runtime_resume_and_get(dsi->dev);
> +	if (ret < 0)
> +		return ret;
>  
> -	return drm_bridge_attach(bridge->encoder, dsi->out_bridge, bridge,
> -				 flags);
> +	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 = {
> 

It seems the right thing to do. But if 'samsung,burst-clock-frequency' is
not specified for DSIM I get a DSI PLL configuration failure. There is no
mode yet, thus samsung_dsim_enable_clock() tries to configure the PLL for
0Hz. There is another reconfiguration while pre_enabling the chain targeting
the correct clock (891000000Hz for 1920x1080), so this seems fine.
But I'm not sure if the 1st config error has any negative side effects.

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

[Marek Vasut]

On 5/16/24 8:51 AM, Alexander Stein wrote:
> Hi Marek,
> 
> thanks for the patch.
> 
> Am Montag, 13. Mai 2024, 04:16:27 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
>> ---
>>   drivers/gpu/drm/bridge/samsung-dsim.c | 16 ++++++++++++++--
>>   1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
>> index 95fedc68b0ae5..56093fc3d62cc 100644
>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>> @@ -1622,9 +1622,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;
>> +
>> +	ret = pm_runtime_resume_and_get(dsi->dev);
>> +	if (ret < 0)
>> +		return ret;
>>   
>> -	return drm_bridge_attach(bridge->encoder, dsi->out_bridge, bridge,
>> -				 flags);
>> +	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 = {
>>
> 
> It seems the right thing to do. But if 'samsung,burst-clock-frequency' is
> not specified for DSIM I get a DSI PLL configuration failure. There is no
> mode yet, thus samsung_dsim_enable_clock() tries to configure the PLL for
> 0Hz. There is another reconfiguration while pre_enabling the chain targeting
> the correct clock (891000000Hz for 1920x1080), so this seems fine.
> But I'm not sure if the 1st config error has any negative side effects.

Will be fixed in V2, thanks for pointing this out.

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

[Alexander Stein]

Hi,

Am Montag, 13. Mai 2024, 04:16:27 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>

This does the trick on my hardware as well.
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>

> ---
> 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
> ---
>  drivers/gpu/drm/bridge/samsung-dsim.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 95fedc68b0ae5..56093fc3d62cc 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -1622,9 +1622,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;
> +
> +	ret = pm_runtime_resume_and_get(dsi->dev);
> +	if (ret < 0)
> +		return ret;
>  
> -	return drm_bridge_attach(bridge->encoder, dsi->out_bridge, bridge,
> -				 flags);
> +	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: [1/2] drm: bridge: samsung-dsim: Initialize bridge on attach

[Marek Vasut]

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

Hello Alexander,

> Am Montag, 13. Mai 2024, 04:16:27 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>
> 
> This does the trick on my hardware as well.
> Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>

Thank you. I still have to address your previous comment on 1/2, I 
didn't get to that one yet, sorry.

Also, I am not sure if this should be applied as-is, it is a borderline 
workaround and there was some discussion about fixing the LP11 lane mode 
switch properly. Michael ?