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 <[email protected]> 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/[email protected]?part=11
