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

[Claudiu Beznea <claudiu.beznea@tuxon.dev>]

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@bp.renesas.com>
>>>>
>>>> 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@bp.renesas.com>
>>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
>>>> ---
>>
>> [ ...]
>>
>>>> +/* -----------------------------------------------------------------------------
>>>> + * 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.

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