Re: [PATCH 3/3] drm: renesas: rz-du: Add support for RZ/G3L LVDS encoder

[Dmitry Baryshkov]

On Tue, Apr 21, 2026 at 12:11:28PM +0300, Claudiu Beznea wrote:
> Hi,
> 
> On 4/19/26 18:58, Dmitry Baryshkov wrote:
> > On Fri, Apr 17, 2026 at 06:52:30PM +0100, Biju wrote:
> > > From: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > > 
> > > Add support for the RZ/G3L LVDS encoder driver. It operates in single-link
> > > mode with 4 lanes (Data) + 1 lane (Clock) and supports pixel clock rates
> > > from 25 to 87 MHz. The LVDS module cannot be used at the same time as
> > > MIPI-DSI. However, LVDS and the DSI interface share a peripheral clock and
> > > the MIPI_DSI_PRESET_N reset signal. Also, the MIPI_DSI_CMN_RSTB and
> > > MIPI_DSI_ARESET_N reset signals must be asserted before using the LVDS
> > > module.
> > > 
> > > Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@xxxxxxxxxxxxxx>
> > > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > > ---
> 
> [ ...]
> 
> > > +/* -----------------------------------------------------------------------------
> > > + * Bridge
> > > + */
> > > +static void rzg3l_lvds_atomic_enable(struct drm_bridge *bridge,
> > > +				     struct drm_atomic_state *state)
> > > +{
> > > +	struct rzg3l_lvds *lvds = bridge_to_rzg3l_lvds(bridge);
> > > +	const struct drm_bridge_state *bridge_state;
> > > +	int ret;
> > > +	u32 fmt;
> > > +
> > > +	/* Get the LVDS format from the bridge state. */
> > > +	bridge_state = drm_atomic_get_new_bridge_state(state, bridge);
> > > +	if (!bridge_state) {
> > > +		dev_err(lvds->dev, "failed to get bridge state\n");
> > > +		return;
> > > +	}
> > > +
> > > +	switch (bridge_state->output_bus_cfg.format) {
> > > +	case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA:
> > > +		fmt = RZG3L_LVDS_MODE_JEIDA;
> > > +		break;
> > > +	case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG:
> > > +		fmt = RZG3L_LVDS_MODE_VESA;
> > > +		break;
> > > +	default:
> > > +		fmt = RZG3L_LVDS_MODE_VESA;
> > > +		dev_warn(lvds->dev, "Unsupported bus fmt 0x%04x\n",
> > > +			 bridge_state->output_bus_cfg.format);
> > > +		break;
> > > +	}
> > > +
> > > +	ret = pm_runtime_resume_and_get(lvds->dev);
> > 
> > If this  fails for any reason, the atomic_disable() would still be
> > called and it will decrement the counter, potentially undeflowing it.
> > Consider switching to pm_runtime_get_sync(), which suits better here.
> 
> AFAIK, the clocks of this HW blocks have MSTOP functionality. HW manual of
> RZ/G3S [1] (should be the same for RZ/G3L as well) mentions the following in
> the chapter 41.2.1. "If the master accesses a module that has the clock
> stopped and the MSTOP bit set, a bus error will occur". [1]
> MSTOP is set though the clock enable/disable APIs.
> 
> The clocks on RZ/G3L are part of clock power domains. If the
> pm_runtime_resume_and_get() fails (or any runtime PM resume calls), the
> clocks will be off and MSTOP set. In this case, calling atomic_disable() or
> any API setting HW registers will lead to sync aborts.

Then you've identified a bug in the code. The atomic_enable() doesn't
fail, so for each enable there always will be an atomic_disable() call.

-- 
With best wishes
Dmitry