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

[Biju Das]

> -----Original Message-----
> From: Claudiu Beznea <claudiu.beznea@xxxxxxxxx>
> Sent: 22 April 2026 09:56
> Subject: Re: [PATCH 3/3] drm: renesas: rz-du: Add support for RZ/G3L LVDS encoder
> 
> 
> 
> 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?

This use case is like system suspend/resume right.

For each system resume call, there will be a system suspend call in future.

Cheers,
Biju