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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

On 7/9/26 14:11, sashiko-bot@kernel.org wrote:
> 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.

None of those are valid, if you want more context, check below.

> --
> 
> --- 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?
> 

That's trolling!!! :-P

Yes, and that's on purpose.

> 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.
> 

It's ok, because it's auto-cleared by HW.

>> @@ -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?

If a legacy device tree is used on MT8196, this MediaTek maintainer (me) needs
a big vacation :-D

This is an impossible scenario.

> 
> 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.
> 

The legacy phy_dev can't be created, unless the DT is very-very-very wrong, and
if it is, this driver can't get a magic 8 ball.

>> +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.
> 

It's not atomic context....