Hi Tommaso Merciai,
> -----Original Message-----
> From: Tommaso Merciai <[email protected]>
> Sent: 17 March 2026 16:05
> Subject: Re: [PATCH 2/2] drm: renesas: rzg2l_mipi_dsi: Fix the power-on
> sequence
>
> Hi Biju,
> Thanks for your patch.
>
> On Tue, Mar 17, 2026 at 12:36:01PM +0000, Biju wrote:
> > From: Biju Das <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > .../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);
> }
OK.
>
>
> > }
> >
> > 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:
NOT ALL.
Only these link registers.
− TXSETR
− ULPSSETR
− DSISETR
− CLSTPTSETR
− LPTRNSTSETR
>
> atomic_pre_enable():
> startup() (F) PHY timing regs + LINK
> set_display_timing() (F) writing VICH1* (LINK regs)
This is not F. This is after starting HS CLK.
> 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)
This is not F. This is after starting HS CLK.
>
>
> 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)
This is not F. It is after starting HSCLK and it is as per hardware manual.
> 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);
.atomic_pre_enable()
dsi_start()
reset_deassert()-->G
.atomic_post_disbale()
Just opposite of atomic_pre_enable()
reset_assert()-->G
dsi_stop()
Cheers,
Biju
