Re: [PATCH 12/12] phy: phy-mtk-dp: Add support for MT8196 eDP PHY

[sashiko-bot@kernel.org]

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

New issues:
- [High] The MT8196 layout places DP_PHYD_LAN_DRIVING_FORCE at an offset that is overwritten by a contiguous bulk write to the driving parameters.

Pre-existing issues:
- [High] Lane disabling logic incorrectly clears the wrong register bits, leaving upper lanes powered on and corrupting adjacent lanes.
- [High] Analog register offsets are mistakenly fetched from the digital register array, causing writes to the wrong address.
- [High] Missing shift for analog calibration parameters passed to regmap_update_bits() zeroes out hardware fields.
--

commit 25701726cdcf8525c87349866f69aa298517187f
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Subject: phy: phy-mtk-dp: Add support for MT8196 eDP PHY

The MT8196 SoC features an updated PHY IP compared to the older ones, and
there is one that is specific to Embedded DisplayPort. This patch adds
support for the eDP PHY found in the MediaTek MT8196 SoC and all of its
variants.

> diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
> index f067acef222f7..998ac34466678 100644
> --- a/drivers/phy/mediatek/phy-mtk-dp.c
> +++ b/drivers/phy/mediatek/phy-mtk-dp.c

[ ... ]

> @@ -178,6 +190,11 @@ static const u8 mt8195_phy_dig_lane_regs[DP_PHYD_LAN_MAX] = {
>  	[DP_PHYD_LAN_DRIVING_PARAM_0] = 0x2c,
>  };
>  
> +static const u8 mt8196_phy_dig_lane_regs[DP_PHYD_LAN_MAX] = {
> +	[DP_PHYD_LAN_DRIVING_FORCE] = 0x30,
> +	[DP_PHYD_LAN_DRIVING_PARAM_0] = 0x2c,
> +};

[Severity: High]
Could this regression corrupt the driving force configuration?

In mtk_dp_phy_set_digital_drv_params(), there is a bulk write of 9 contiguous
32-bit registers starting at DP_PHYD_LAN_DRIVING_PARAM_0:

    ret = regmap_bulk_write(dp_phy->regmap,
                            pdata->off_dig_lane[i] + reg,
                            pdata->driving_params,
                            ARRAY_SIZE(pdata->driving_params));

Since the bulk write starts at 0x2c, driving_params[1] will be written to 0x30,
which overlaps with DP_PHYD_LAN_DRIVING_FORCE.

> +
>  static const u8 mt8195_phy_dig_glb_regs[DP_PHYD_GLOBAL_MAX] = {

[ ... ]

> @@ -751,8 +784,39 @@ static const struct mtk_dp_phy_pdata mt8195_dp_phy_data = {
>  	},
>  };
>  
> +static const struct mtk_dp_phy_pdata mt8196_edp_phy_data = {
> +	.off_ana_glb = 0x400,
> +	.off_ana_lane = (const u16[]) { 0x0, 0x100, 0x200, 0x300 },
> +	.off_dig_glb = 0x1400,
> +	.off_dig_lane = (const u16[]) { 0x1000, 0x1100, 0x1200, 0x1300 },
> +	.regs_ana_glb = mt8195_phy_ana_glb_regs,
> +	.regs_ana_lane = mt8195_phy_ana_lane_regs,
> +	.regs_dig_glb = mt8196_phy_dig_glb_regs,

[Severity: High]
This is a pre-existing issue, but do mtk_dp_phy_power_on() and
mtk_dp_phy_power_off() fetch analog register offsets using this digital register
array?

Both functions use the analog enum DP_PHYA_GLB_FORCE_CTRL_1 to access the
digital array:

    const u8 *regs = pdata->regs_dig_glb;
    ...
    ret = regmap_set_bits(dp_phy->regmap,
                          pdata->off_ana_glb + regs[DP_PHYA_GLB_FORCE_CTRL_1],
                          CKM_CKTX0_EN_FORCE_MODE);

Accessing index 4 (DP_PHYA_GLB_FORCE_CTRL_1) of the digital array returns
DP_PHYD_AUX_RX_CTL (0x40) instead of the intended analog offset (0x34).

Does this overwrite incorrect memory mapped register offsets?

> +	.regs_dig_lane = mt8196_phy_dig_lane_regs,
> +	.mask_dig_tx_ln = PHYD_TX_LN_EN_V2,

[Severity: High]
This isn't a bug introduced by this patch, but does the lane disabling logic in
mtk_dp_phy_disable_all_lanes() use this mask incorrectly?

It computes lane_num as a raw index and passes it to field_prep():

    ret = regmap_clear_bits(dp_phy->regmap,
                            pdata->off_dig_glb + regs[DP_PHYD_TX_CTL_0],
                            field_prep(pdata->mask_dig_tx_ln, lane_num));

Because field_prep() shifts the raw value, passing 3 instead of BIT(3) will
shift 0b11 into the mask, clearing bits 4 and 5 instead of bit 7 (or bits 0 and
1 instead of bit 3 for V2).

Should this be using BIT(lane_num) instead?

> +	.val_dig_bitrate = mt8196_edp_phy_dig_bitrate_val,
> +	.ana_bias_r = 15,
> +	.ana_cktx_imp = 8,

[Severity: High]
This isn't a bug introduced by this patch, but are these calibration parameters
unconditionally cleared to zero in the hardware?

In mtk_dp_phy_set_analog_calibration_params(), these values are passed
directly to regmap_update_bits():

    ret = regmap_update_bits(dp_phy->regmap,
                             pdata->off_ana_glb + regs_ana_glb[DP_PHYA_GLB_BIAS_GEN_0],
                             XTP_GLB_BIAS_INT_R_CTRL, pdata->ana_bias_r);

Because regmap_update_bits() expects a pre-shifted value and applies a mask
(e.g., GENMASK(20, 16)), an unshifted raw value like 15 masked against the upper
bits yields 0.

Should a macro like FIELD_PREP() be used to properly shift these values into
place?

> +	.ana_lanes_imp = {
> +		.pmos = 8,
> +		.nmos = 8,
> +	},

[ ... ]

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