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

[Dmitry Baryshkov]

On Wed, Apr 22, 2026 at 11:55:37AM +0300, Claudiu Beznea wrote:
> 
> 
> On 4/21/26 14:22, Dmitry Baryshkov wrote:
> > 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.
> > 
> 
> Is this something that should be solved by individual drivers providing
> struct drm_bridge_funcs to the upper layers or by the subsystem itself?
> 
> Accessing HW w/o its power being on (whatever power means here, e.g. clocks,
> resets, regulators) seems odd and may lead to critical failures.

The major problem is that atomic_enable() can't fail for the design
reasons. So, yes, currently this needs to be handled by the driver which
calls pm_resume in the atomic_enable() path.

> 
> On some Renesas SoCs this used to work previously but it is not anymore with
> the addition of the so called MSTOP functionality.
> 
> Thank you,
> Claudiu

-- 
With best wishes
Dmitry