Re: [PATCH 2/2] drm: renesas: rzg2l_mipi_dsi: Fix the power-on sequence

[Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>]

Hi Biju,
Thanks for your patch.

On Tue, Mar 17, 2026 at 12:36:01PM +0000, Biju wrote:
> From: Biju Das <biju.das.jz@bp.renesas.com>
> 
> Move reset_control_deassert() and reset_control_assert() from
> rzg2l_mipi_dsi_dphy_init()/rzg2l_mipi_dsi_dphy_exit() to
> atomic_pre_enable() and atomic_post_disable() respectively, and move
> rzg2l_mipi_dsi_set_display_timing() from atomic_pre_enable() to
> atomic_enable(), to align with the power-on sequence described in
> Figure 34.5 of section "34.4.2.1 Reset" of the RZ/G2L hardware manual
> Rev.1.50 May 2025.
> 
> According to the hardware manual, LINK registers must be written before
> deasserting CMN_RSTB, and the 1ms delay is retained in atomic_pre_enable()
> after the deassert.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>  .../gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c    | 27 +++++++++++--------
>  1 file changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c b/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c
> index e53b48e4de56..9053ce037b75 100644
> --- a/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c
> +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c
> @@ -484,7 +484,6 @@ static int rzg2l_mipi_dsi_dphy_init(struct rzg2l_mipi_dsi *dsi,
>  	u32 dphytim1;
>  	u32 dphytim2;
>  	u32 dphytim3;
> -	int ret;
>  
>  	/* All DSI global operation timings are set with recommended setting */
>  	for (i = 0; i < ARRAY_SIZE(rzg2l_mipi_dsi_global_timings); ++i) {
> @@ -524,12 +523,6 @@ static int rzg2l_mipi_dsi_dphy_init(struct rzg2l_mipi_dsi *dsi,
>  	rzg2l_mipi_dsi_phy_write(dsi, DSIDPHYTIM2, dphytim2);
>  	rzg2l_mipi_dsi_phy_write(dsi, DSIDPHYTIM3, dphytim3);
>  
> -	ret = reset_control_deassert(dsi->rstc);
> -	if (ret < 0)
> -		return ret;
> -
> -	fsleep(1000);
> -
>  	return 0;
>  }
>  
> @@ -541,8 +534,6 @@ static void rzg2l_mipi_dsi_dphy_exit(struct rzg2l_mipi_dsi *dsi)
>  
>  	dphyctrl0 &= ~(DSIDPHYCTRL0_EN_LDO1200 | DSIDPHYCTRL0_EN_BGR);
>  	rzg2l_mipi_dsi_phy_write(dsi, DSIDPHYCTRL0, dphyctrl0);
> -
> -	reset_control_assert(dsi->rstc);
>  }
>  
>  static int rzg2l_dphy_conf_clks(struct rzg2l_mipi_dsi *dsi, unsigned long mode_freq,
> @@ -1030,24 +1021,37 @@ static void rzg2l_mipi_dsi_atomic_pre_enable(struct drm_bridge *bridge,
>  	connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
>  	crtc = drm_atomic_get_new_connector_state(state, connector)->crtc;
>  	mode = &drm_atomic_get_new_crtc_state(state, crtc)->adjusted_mode;
> -
>  	ret = rzg2l_mipi_dsi_startup(dsi, mode);
>  	if (ret < 0)
>  		return;
>  
> -	rzg2l_mipi_dsi_set_display_timing(dsi, mode);
> +	ret = reset_control_deassert(dsi->rstc);
> +	if (ret < 0)
> +		return;
> +
> +	if (dsi->rstc)
> +		fsleep(1000);

What about?

	if (dsi->rstc) {
	    ret = reset_control_deassert(dsi->rstc);
	    if (ret < 0)
		return;

	    fsleep(1000);
	}


>  }
>  
>  static void rzg2l_mipi_dsi_atomic_enable(struct drm_bridge *bridge,
>  					 struct drm_atomic_state *state)
>  {
>  	struct rzg2l_mipi_dsi *dsi = bridge_to_rzg2l_mipi_dsi(bridge);
> +	const struct drm_display_mode *mode;
> +	struct drm_connector *connector;
> +	struct drm_crtc *crtc;
>  	int ret;
>  
>  	ret = rzg2l_mipi_dsi_start_hs_clock(dsi);
>  	if (ret < 0)
>  		goto err_stop;
>  
> +	connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
> +	crtc = drm_atomic_get_new_connector_state(state, connector)->crtc;
> +	mode = &drm_atomic_get_new_crtc_state(state, crtc)->adjusted_mode;
> +
> +	rzg2l_mipi_dsi_set_display_timing(dsi, mode);
> +

Manual/Patch says that LINK registers must be written before
deasserting CMN_RSTB:

  atomic_pre_enable():
	  startup()                  (F) PHY timing regs + LINK
	  set_display_timing()       (F) writing VICH1* (LINK regs)
	  reset_control_deassert()   (G)
	  fsleep(1000)               (H)

Before this series we have:

  atomic_pre_enable():
    startup()
      dphy_init()
        write DSIDPHYTIMx         (F) PHY timing regs
        reset_control_deassert()  (G) deassert CMN_RSTB
        udelay(1)                 (H)
    set_display_timing()          (F) writing VICH1* (LINK regs)  


Moving set_display_timing() here you are setting LINK regs after
reset_control_deassert() and the sequence will be:

 atomic_pre_enable():
	 startup()		  (F) PHY timing regs + LINK
	 reset_control_deassert() (G) CMN_RSTB deassert
	 fsleep(1000)             (H) wait 1ms

 atomic_enable():
	 start_hs_clock()
	 set_display_timing()     (F) writing VICH1* (LINK regs)
	 start_video()

I think to provide the right sequence we need to just move

	reset_control_deassert(dsi->rstc);

From rzg2l_mipi_dsi_dphy_init() to rzg2l_mipi_dsi_atomic_pre_enable()
just after rzg2l_mipi_dsi_set_display_timing() call.


>  	ret = rzg2l_mipi_dsi_start_video(dsi);
>  	if (ret < 0)
>  		goto err_stop_clock;
> @@ -1074,6 +1078,7 @@ static void rzg2l_mipi_dsi_atomic_post_disable(struct drm_bridge *bridge,
>  {
>  	struct rzg2l_mipi_dsi *dsi = bridge_to_rzg2l_mipi_dsi(bridge);
>  
> +	reset_control_assert(dsi->rstc);
>  	rzg2l_mipi_dsii_stop(dsi);

rzg2l_mipi_dsi_stop() is writing DSIDPHYCTRL0 reg via dphy_exit().
I think the right order should be:

	rzg2l_mipi_dsii_stop(dsi);
	reset_control_assert(dsi->rstc);

What do you think?


Kind Regards,
Tommaso

>  }
>  
> -- 
> 2.43.0
>