diff mbox series

[01/20] drm/bridge: add dsi_lp11_notify mechanism

Message ID 20240506-tc358775-fix-powerup-v1-1-545dcf00b8dd@kernel.org (mailing list archive)
State New, archived
Headers show
Series drm/bridge: tc358775: proper bridge bringup and code cleanup | expand

Commit Message

Michael Walle May 6, 2024, 1:34 p.m. UTC
Some bridges have very strict power-up reqirements. In this case, the
Toshiba TC358775. The reset has to be deasserted while *both* the DSI
clock and DSI data lanes are in LP-11 mode. After the reset is relased,
the bridge needs the DSI clock to actually be able to process I2C
access. This access will configure the DSI side of the bridge during
which the DSI data lanes have to be in LP-11 mode. After everything is
configured the video stream can finally be enabled.

This means:
 (1) The bridge has to be configured completely in .pre_enable() op
     (with the clock turned on and data lanes in LP-11 mode, thus
     .pre_enable_prev_first has to be set).
 (2) The bridge will enable its output in the .enable() op
 (3) There must be some mechanism before (1) where the bridge can
     release its reset while the clock lane is still in LP-11 mode.

Unfortunately, (3) is crucial for a correct operation of the bridge.
To satisfy this requriment, introduce a new callback .dsi_lp11_notify()
which will be called by the DSI host driver.

Signed-off-by: Michael Walle <mwalle@kernel.org>
---
 drivers/gpu/drm/drm_bridge.c | 16 ++++++++++++++++
 include/drm/drm_bridge.h     | 12 ++++++++++++
 2 files changed, 28 insertions(+)

Comments

Alexander Stein May 7, 2024, 8:37 a.m. UTC | #1
Hi Michael,

Am Montag, 6. Mai 2024, 15:34:30 CEST schrieb Michael Walle:
> Some bridges have very strict power-up reqirements. In this case, the
> Toshiba TC358775. The reset has to be deasserted while *both* the DSI
> clock and DSI data lanes are in LP-11 mode. After the reset is relased,
> the bridge needs the DSI clock to actually be able to process I2C
> access. This access will configure the DSI side of the bridge during
> which the DSI data lanes have to be in LP-11 mode.

Apparently this is an issue for a lot of DSI bridges. But enabling LP-11 for
a bridge is impossible with current documentation [1], which states "A DSI
host should keep the PHY powered down until the pre_enable operation is
called."
Additionally tc358767/tc9595 (DSI-DP bridge) needs LP-11 for AUX channel
access to even get EDID. This is a requirement before pre_enable would
even be possible.

So some changes to the current flow are needed. But I am not so sure
about LP-11 notification. IMHO a device request to the DSI host to
enable LP-11 seems more sensible.

Best regards,
Alexander

[1] https://www.kernel.org/doc/html/latest/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation

> After everything is
> configured the video stream can finally be enabled.
> 
> This means:
>  (1) The bridge has to be configured completely in .pre_enable() op
>      (with the clock turned on and data lanes in LP-11 mode, thus
>      .pre_enable_prev_first has to be set).
>  (2) The bridge will enable its output in the .enable() op
>  (3) There must be some mechanism before (1) where the bridge can
>      release its reset while the clock lane is still in LP-11 mode.
> 
> Unfortunately, (3) is crucial for a correct operation of the bridge.
> To satisfy this requriment, introduce a new callback .dsi_lp11_notify()
> which will be called by the DSI host driver.
> 
> Signed-off-by: Michael Walle <mwalle@kernel.org>
> ---
>  drivers/gpu/drm/drm_bridge.c | 16 ++++++++++++++++
>  include/drm/drm_bridge.h     | 12 ++++++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 28abe9aa99ca..98cd6558aecb 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -1339,6 +1339,22 @@ void drm_bridge_hpd_notify(struct drm_bridge *bridge,
>  }
>  EXPORT_SYMBOL_GPL(drm_bridge_hpd_notify);
>  
> +/**
> + * drm_bridge_dsi_lp11_notify - notify clock/data lanes LP-11 mode
> + * @bridge: bridge control structure
> + *
> + * DSI host drivers shall call this function while the clock and data lanes
> + * are still in LP-11 mode.
> + *
> + * This function shall be called in a context that can sleep.
> + */
> +void drm_bridge_dsi_lp11_notify(struct drm_bridge *bridge)
> +{
> +	if (bridge->funcs->dsi_lp11_notify)
> +		bridge->funcs->dsi_lp11_notify(bridge);
> +}
> +EXPORT_SYMBOL_GPL(drm_bridge_dsi_lp11_notify);
> +
>  #ifdef CONFIG_OF
>  /**
>   * of_drm_find_bridge - find the bridge corresponding to the device node in
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 4baca0d9107b..4ef61274e0a8 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -630,6 +630,17 @@ struct drm_bridge_funcs {
>  	 */
>  	void (*hpd_disable)(struct drm_bridge *bridge);
>  
> +	/**
> +	 * dsi_lp11_notify:
> +	 *
> +	 * Will be called by the DSI host driver while both the DSI clock
> +	 * lane as well as the DSI data lanes are in LP-11 mode. Some bridges
> +	 * need this state while releasing the reset, for example.
> +	 * Not all DSI host drivers will support this. Therefore, the DSI
> +	 * bridge driver must not rely on this op to be called.
> +	 */
> +	void (*dsi_lp11_notify)(struct drm_bridge *bridge);
> +
>  	/**
>  	 * @debugfs_init:
>  	 *
> @@ -898,6 +909,7 @@ void drm_bridge_hpd_enable(struct drm_bridge *bridge,
>  void drm_bridge_hpd_disable(struct drm_bridge *bridge);
>  void drm_bridge_hpd_notify(struct drm_bridge *bridge,
>  			   enum drm_connector_status status);
> +void drm_bridge_dsi_lp11_notify(struct drm_bridge *bridge);
>  
>  #ifdef CONFIG_DRM_PANEL_BRIDGE
>  bool drm_bridge_is_panel(const struct drm_bridge *bridge);
> 
>
Dmitry Baryshkov May 7, 2024, 1:39 p.m. UTC | #2
On Tue, May 07, 2024 at 10:37:54AM +0200, Alexander Stein wrote:
> Hi Michael,
> 
> Am Montag, 6. Mai 2024, 15:34:30 CEST schrieb Michael Walle:
> > Some bridges have very strict power-up reqirements. In this case, the
> > Toshiba TC358775. The reset has to be deasserted while *both* the DSI
> > clock and DSI data lanes are in LP-11 mode. After the reset is relased,
> > the bridge needs the DSI clock to actually be able to process I2C
> > access. This access will configure the DSI side of the bridge during
> > which the DSI data lanes have to be in LP-11 mode.
> 
> Apparently this is an issue for a lot of DSI bridges. But enabling LP-11 for
> a bridge is impossible with current documentation [1], which states "A DSI
> host should keep the PHY powered down until the pre_enable operation is
> called."
> Additionally tc358767/tc9595 (DSI-DP bridge) needs LP-11 for AUX channel
> access to even get EDID. This is a requirement before pre_enable would
> even be possible.
> 
> So some changes to the current flow are needed. But I am not so sure
> about LP-11 notification. IMHO a device request to the DSI host to
> enable LP-11 seems more sensible.

Granted that there can be several DSI devices sharing the DSI bus (aka
split-link), I was toying with the idea of making the DSI host call
attached DSI devices when the transition happens.

I don't have a fully working PoC and I probably won't have it ready til
the end of May because of the lack of time and different local
priorities.

> 
> Best regards,
> Alexander
> 
> [1] https://www.kernel.org/doc/html/latest/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
> 
> > After everything is
> > configured the video stream can finally be enabled.
> > 
> > This means:
> >  (1) The bridge has to be configured completely in .pre_enable() op
> >      (with the clock turned on and data lanes in LP-11 mode, thus
> >      .pre_enable_prev_first has to be set).
> >  (2) The bridge will enable its output in the .enable() op
> >  (3) There must be some mechanism before (1) where the bridge can
> >      release its reset while the clock lane is still in LP-11 mode.
> > 
> > Unfortunately, (3) is crucial for a correct operation of the bridge.
> > To satisfy this requriment, introduce a new callback .dsi_lp11_notify()
> > which will be called by the DSI host driver.
> > 
> > Signed-off-by: Michael Walle <mwalle@kernel.org>
> > ---
> >  drivers/gpu/drm/drm_bridge.c | 16 ++++++++++++++++
> >  include/drm/drm_bridge.h     | 12 ++++++++++++
> >  2 files changed, 28 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index 28abe9aa99ca..98cd6558aecb 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -1339,6 +1339,22 @@ void drm_bridge_hpd_notify(struct drm_bridge *bridge,
> >  }
> >  EXPORT_SYMBOL_GPL(drm_bridge_hpd_notify);
> >  
> > +/**
> > + * drm_bridge_dsi_lp11_notify - notify clock/data lanes LP-11 mode
> > + * @bridge: bridge control structure
> > + *
> > + * DSI host drivers shall call this function while the clock and data lanes
> > + * are still in LP-11 mode.
> > + *
> > + * This function shall be called in a context that can sleep.
> > + */
> > +void drm_bridge_dsi_lp11_notify(struct drm_bridge *bridge)
> > +{
> > +	if (bridge->funcs->dsi_lp11_notify)
> > +		bridge->funcs->dsi_lp11_notify(bridge);
> > +}
> > +EXPORT_SYMBOL_GPL(drm_bridge_dsi_lp11_notify);
> > +
> >  #ifdef CONFIG_OF
> >  /**
> >   * of_drm_find_bridge - find the bridge corresponding to the device node in
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index 4baca0d9107b..4ef61274e0a8 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -630,6 +630,17 @@ struct drm_bridge_funcs {
> >  	 */
> >  	void (*hpd_disable)(struct drm_bridge *bridge);
> >  
> > +	/**
> > +	 * dsi_lp11_notify:
> > +	 *
> > +	 * Will be called by the DSI host driver while both the DSI clock
> > +	 * lane as well as the DSI data lanes are in LP-11 mode. Some bridges
> > +	 * need this state while releasing the reset, for example.
> > +	 * Not all DSI host drivers will support this. Therefore, the DSI
> > +	 * bridge driver must not rely on this op to be called.
> > +	 */
> > +	void (*dsi_lp11_notify)(struct drm_bridge *bridge);
> > +
> >  	/**
> >  	 * @debugfs_init:
> >  	 *
> > @@ -898,6 +909,7 @@ void drm_bridge_hpd_enable(struct drm_bridge *bridge,
> >  void drm_bridge_hpd_disable(struct drm_bridge *bridge);
> >  void drm_bridge_hpd_notify(struct drm_bridge *bridge,
> >  			   enum drm_connector_status status);
> > +void drm_bridge_dsi_lp11_notify(struct drm_bridge *bridge);
> >  
> >  #ifdef CONFIG_DRM_PANEL_BRIDGE
> >  bool drm_bridge_is_panel(const struct drm_bridge *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/
> 
>
Michael Walle June 3, 2024, 1:39 p.m. UTC | #3
[+ Marek ]

Hi Dmitry,

> > > Some bridges have very strict power-up reqirements. In this case, the
> > > Toshiba TC358775. The reset has to be deasserted while *both* the DSI
> > > clock and DSI data lanes are in LP-11 mode. After the reset is relased,
> > > the bridge needs the DSI clock to actually be able to process I2C
> > > access. This access will configure the DSI side of the bridge during
> > > which the DSI data lanes have to be in LP-11 mode.
> > 
> > Apparently this is an issue for a lot of DSI bridges. But enabling LP-11 for
> > a bridge is impossible with current documentation [1], which states "A DSI
> > host should keep the PHY powered down until the pre_enable operation is
> > called."
> > Additionally tc358767/tc9595 (DSI-DP bridge) needs LP-11 for AUX channel
> > access to even get EDID. This is a requirement before pre_enable would
> > even be possible.
> > 
> > So some changes to the current flow are needed. But I am not so sure
> > about LP-11 notification. IMHO a device request to the DSI host to
> > enable LP-11 seems more sensible.
>
> Granted that there can be several DSI devices sharing the DSI bus (aka
> split-link), I was toying with the idea of making the DSI host call
> attached DSI devices when the transition happens.

So almost the same, as this patch?

> I don't have a fully working PoC and I probably won't have it ready til
> the end of May because of the lack of time and different local
> priorities.

Any news regarding this?

-michael

> > Best regards,
> > Alexander
> > 
> > [1] https://www.kernel.org/doc/html/latest/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
> > 
> > > After everything is
> > > configured the video stream can finally be enabled.
> > > 
> > > This means:
> > >  (1) The bridge has to be configured completely in .pre_enable() op
> > >      (with the clock turned on and data lanes in LP-11 mode, thus
> > >      .pre_enable_prev_first has to be set).
> > >  (2) The bridge will enable its output in the .enable() op
> > >  (3) There must be some mechanism before (1) where the bridge can
> > >      release its reset while the clock lane is still in LP-11 mode.
> > > 
> > > Unfortunately, (3) is crucial for a correct operation of the bridge.
> > > To satisfy this requriment, introduce a new callback .dsi_lp11_notify()
> > > which will be called by the DSI host driver.
> > > 
> > > Signed-off-by: Michael Walle <mwalle@kernel.org>
> > > ---
> > >  drivers/gpu/drm/drm_bridge.c | 16 ++++++++++++++++
> > >  include/drm/drm_bridge.h     | 12 ++++++++++++
> > >  2 files changed, 28 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > > index 28abe9aa99ca..98cd6558aecb 100644
> > > --- a/drivers/gpu/drm/drm_bridge.c
> > > +++ b/drivers/gpu/drm/drm_bridge.c
> > > @@ -1339,6 +1339,22 @@ void drm_bridge_hpd_notify(struct drm_bridge *bridge,
> > >  }
> > >  EXPORT_SYMBOL_GPL(drm_bridge_hpd_notify);
> > >  
> > > +/**
> > > + * drm_bridge_dsi_lp11_notify - notify clock/data lanes LP-11 mode
> > > + * @bridge: bridge control structure
> > > + *
> > > + * DSI host drivers shall call this function while the clock and data lanes
> > > + * are still in LP-11 mode.
> > > + *
> > > + * This function shall be called in a context that can sleep.
> > > + */
> > > +void drm_bridge_dsi_lp11_notify(struct drm_bridge *bridge)
> > > +{
> > > +	if (bridge->funcs->dsi_lp11_notify)
> > > +		bridge->funcs->dsi_lp11_notify(bridge);
> > > +}
> > > +EXPORT_SYMBOL_GPL(drm_bridge_dsi_lp11_notify);
> > > +
> > >  #ifdef CONFIG_OF
> > >  /**
> > >   * of_drm_find_bridge - find the bridge corresponding to the device node in
> > > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > > index 4baca0d9107b..4ef61274e0a8 100644
> > > --- a/include/drm/drm_bridge.h
> > > +++ b/include/drm/drm_bridge.h
> > > @@ -630,6 +630,17 @@ struct drm_bridge_funcs {
> > >  	 */
> > >  	void (*hpd_disable)(struct drm_bridge *bridge);
> > >  
> > > +	/**
> > > +	 * dsi_lp11_notify:
> > > +	 *
> > > +	 * Will be called by the DSI host driver while both the DSI clock
> > > +	 * lane as well as the DSI data lanes are in LP-11 mode. Some bridges
> > > +	 * need this state while releasing the reset, for example.
> > > +	 * Not all DSI host drivers will support this. Therefore, the DSI
> > > +	 * bridge driver must not rely on this op to be called.
> > > +	 */
> > > +	void (*dsi_lp11_notify)(struct drm_bridge *bridge);
> > > +
> > >  	/**
> > >  	 * @debugfs_init:
> > >  	 *
> > > @@ -898,6 +909,7 @@ void drm_bridge_hpd_enable(struct drm_bridge *bridge,
> > >  void drm_bridge_hpd_disable(struct drm_bridge *bridge);
> > >  void drm_bridge_hpd_notify(struct drm_bridge *bridge,
> > >  			   enum drm_connector_status status);
> > > +void drm_bridge_dsi_lp11_notify(struct drm_bridge *bridge);
> > >  
> > >  #ifdef CONFIG_DRM_PANEL_BRIDGE
> > >  bool drm_bridge_is_panel(const struct drm_bridge *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/
> > 
> >
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 28abe9aa99ca..98cd6558aecb 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -1339,6 +1339,22 @@  void drm_bridge_hpd_notify(struct drm_bridge *bridge,
 }
 EXPORT_SYMBOL_GPL(drm_bridge_hpd_notify);
 
+/**
+ * drm_bridge_dsi_lp11_notify - notify clock/data lanes LP-11 mode
+ * @bridge: bridge control structure
+ *
+ * DSI host drivers shall call this function while the clock and data lanes
+ * are still in LP-11 mode.
+ *
+ * This function shall be called in a context that can sleep.
+ */
+void drm_bridge_dsi_lp11_notify(struct drm_bridge *bridge)
+{
+	if (bridge->funcs->dsi_lp11_notify)
+		bridge->funcs->dsi_lp11_notify(bridge);
+}
+EXPORT_SYMBOL_GPL(drm_bridge_dsi_lp11_notify);
+
 #ifdef CONFIG_OF
 /**
  * of_drm_find_bridge - find the bridge corresponding to the device node in
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 4baca0d9107b..4ef61274e0a8 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -630,6 +630,17 @@  struct drm_bridge_funcs {
 	 */
 	void (*hpd_disable)(struct drm_bridge *bridge);
 
+	/**
+	 * dsi_lp11_notify:
+	 *
+	 * Will be called by the DSI host driver while both the DSI clock
+	 * lane as well as the DSI data lanes are in LP-11 mode. Some bridges
+	 * need this state while releasing the reset, for example.
+	 * Not all DSI host drivers will support this. Therefore, the DSI
+	 * bridge driver must not rely on this op to be called.
+	 */
+	void (*dsi_lp11_notify)(struct drm_bridge *bridge);
+
 	/**
 	 * @debugfs_init:
 	 *
@@ -898,6 +909,7 @@  void drm_bridge_hpd_enable(struct drm_bridge *bridge,
 void drm_bridge_hpd_disable(struct drm_bridge *bridge);
 void drm_bridge_hpd_notify(struct drm_bridge *bridge,
 			   enum drm_connector_status status);
+void drm_bridge_dsi_lp11_notify(struct drm_bridge *bridge);
 
 #ifdef CONFIG_DRM_PANEL_BRIDGE
 bool drm_bridge_is_panel(const struct drm_bridge *bridge);