Re: [PATCH v4 11/12] drm/mediatek: mtk_dp: Add support for eDP1.5 IPs and MT8196 SoC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Flawed hardware reset sequence and register mask conflict for the new IP (hwip_v1p4) in mtk_dp_encoder_reset().
- [High] Uninitialized calibration data is applied to PHY registers on MT8196 if a legacy device tree is used.

Pre-existing issues:
- [High] Sleeping functions called from atomic context in bridge atomic callbacks.
--

--- Patch [11]: [PATCH v4 11/12] drm/mediatek: mtk_dp: Add support for eDP1.5 IPs and MT8196 SoC ---
commit 21e6cb76ec5d736e41c62351d0dcc4f501bc644f
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

drm/mediatek: mtk_dp: Add support for eDP1.5 IPs and MT8196 SoC

Add support for MediaTek's Embedded DisplayPort (eDP) v1.5 IP as
found in the MT8196 SoC and other variants, and enclose some of
the new register writes in specific platform data booleans.

> --- a/drivers/gpu/drm/mediatek/mtk_dp_reg.h
> +++ b/drivers/gpu/drm/mediatek/mtk_dp_reg.h
[ ... ]
> +#define REG_3FF8_DP_ENC_P0_3			0x1ff8
> +#define ENC_PHY_STATE_RESET_ALL_MASK			GENMASK(7, 0)
> +#define PHY_STATE_W_1_DP_ENC_P0_3			BIT(6)
> +#define DP_ENCODER_DVO_ON_W_1				BIT(5)
> +#define DP_ENCODER_PSR_XTAL_FREQ_MHZ_MASK		GENMASK(13, 9)

[ ... ]

> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
[ ... ]
> +static void mtk_dp_encoder_reset(struct mtk_dp *mtk_dp)
> +{
>  	/* dp tx encoder reset all sw */
>  	mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3004,
>  			   DP_TX_ENCODER_4P_RESET_SW_DP_ENC0_P0,
>  			   DP_TX_ENCODER_4P_RESET_SW_DP_ENC0_P0);
>  
> +	if (mtk_dp->data->hwip_v1p4) {
> +		mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3004, 0,
> +				   DP_TX_ENCODER_4P_RESET_SW_DP_ENC0_P0);
> +		mtk_dp_update_bits(mtk_dp, REG_3FF8_DP_ENC_P0_3,
> +				   ENC_PHY_STATE_RESET_ALL_MASK,
> +				   ENC_PHY_STATE_RESET_ALL_MASK);
> +	}
> +
>  	/* Wait for sw reset to complete */
>  	usleep_range(1000, 5000);
> -	mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3004,
> -			   0, DP_TX_ENCODER_4P_RESET_SW_DP_ENC0_P0);
> +
> +	if (!mtk_dp->data->hwip_v1p4)
> +		mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3004,
> +				   0, DP_TX_ENCODER_4P_RESET_SW_DP_ENC0_P0);
> +}

[Severity: High]
Does ENC_PHY_STATE_RESET_ALL_MASK overlap with functional state bits
like PHY_STATE_W_1_DP_ENC_P0_3 and DP_ENCODER_DVO_ON_W_1 defined in
mtk_dp_reg.h?

Also, could the new hwip_v1p4 sequence in mtk_dp_encoder_reset() leave the
encoder PHY stuck in reset? The code asserts ENC_PHY_STATE_RESET_ALL_MASK
before the wait, but there does not appear to be a step to deassert it after
the sleep finishes.

> @@ -2860,7 +3079,8 @@ static int mtk_dp_register_phy(struct mtk_dp *mtk_dp)
[ ... ]
>  	/* PHY calibration data is in mtk_dp only for legacy devicetree */
> -	mtk_dp_get_calibration_data(mtk_dp);
> +	if (mtk_dp->data->efuse_fmt)
> +		mtk_dp_get_calibration_data(mtk_dp);
>  
>  	mtk_dp->phy = devm_phy_get(&mtk_dp->phy_dev->dev, "dp");

[ ... ]

> @@ -1359,8 +1550,10 @@ static int mtk_dp_phy_configure(struct mtk_dp *mtk_dp,
[ ... ]
>  	/*
>  	 * For legacy, deprecated strategy, set partial PHY calibration here.
>  	 * New-style will set all PHY calibrations with phy ops instead.
>  	 */
>  	if (mtk_dp->phy_dev)
>  		mtk_dp_set_calibration_data(mtk_dp);

[Severity: High]
If a legacy device tree is used on an MT8196 device, could this configuration
apply uninitialized zeros to the PHY hardware?

In mtk_dp_register_phy(), mtk_dp_get_calibration_data() is skipped because
efuse_fmt is NULL for mt8196_edp_data. However, mtk_dp_phy_configure() will
still call mtk_dp_set_calibration_data() if the legacy phy_dev was created,
potentially writing an unpopulated cal_data array directly to the registers.

> +static void mtk_dp_encoder_force_bias_power_on(struct mtk_dp *mtk_dp)
> +{
[ ... ]
> +	/* Wait for the MCU to get LDO ready in PHYD */
> +	msleep(50);
[ ... ]
> +}

[ ... ]

> @@ -2580,6 +2795,10 @@ static void mtk_dp_bridge_atomic_disable(struct drm_bridge *bridge,
[ ... ]
>  	/* SDP path reset sw*/
>  	mtk_dp_sdp_path_reset(mtk_dp);
>  
> +	/* Make sure BIAS is powered on after reset even when disabled */
> +	if (mtk_dp->data->force_pwr_states)
> +		mtk_dp_encoder_force_bias_power_on(mtk_dp);
> +
>  	/* Ensure the sink is muted */
>  	msleep(20);
>  }

[Severity: High]
This is a pre-existing issue, but does this introduce an additional sleeping
function into an atomic context path?

Since mtk_dp_bridge_atomic_disable() operates in an atomic context during
modesetting, calling mtk_dp_encoder_force_bias_power_on() uses msleep(50),
which could result in kernel warnings or system instability.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709113148.49090-1-angelogioacchino.delregno@collabora.com?part=11