[PATCH v13 05/24] phy: mediatek: ufs: Add support for resets

[sashiko-bot at kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Strict probe failure when firmware lacks optional MPHY backup SMC
- [Medium] Unhandled firmware error codes are silently treated as success in ufs_mtk_phy_ctrl()
--

commit 9009c477dc0a8eefd792f29d6be7589214adb09e
Author: Nicolas Frattaroli <nicolas.frattaroli at collabora.com>

phy: mediatek: ufs: Add support for resets

The MediaTek UFS PHY supports PHY resets. Until now, they've been
implemented in the UFS host driver. This commit migrates the MPHY reset logic
into the PHY driver using the PHY subsystem's reset op.

> diff --git a/drivers/phy/mediatek/phy-mtk-ufs.c b/drivers/phy/mediatek/phy-mtk-ufs.c
> index fc19e0fa8ed55..4439a9eb6b7ad 100644
> --- a/drivers/phy/mediatek/phy-mtk-ufs.c
> +++ b/drivers/phy/mediatek/phy-mtk-ufs.c
[ ... ]
> @@ -140,9 +151,59 @@ static int ufs_mtk_phy_power_off(struct phy *generic_phy)
>  	return 0;
>  }
>  
> +static int ufs_mtk_phy_ctrl(struct ufs_mtk_phy *phy, enum ufs_mtk_mphy_op op)
> +{
> +	struct arm_smccc_res res;
> +
> +	arm_smccc_smc(MTK_SIP_UFS_CONTROL, UFS_MTK_SIP_MPHY_CTRL, op,
> +		      0, 0, 0, 0, 0, &res);
> +
> +	switch (res.a0) {
> +	case SMCCC_RET_NOT_SUPPORTED:
> +		return -EOPNOTSUPP;
> +	case SMCCC_RET_INVALID_PARAMETER:
> +		return -EINVAL;
> +	default:
> +		return 0;
> +	}

[Severity: Medium]
Does this default case mask unhandled firmware error codes?

If the firmware encounters a failure during PHY backup and restore operations
and returns an error code other than SMCCC_RET_NOT_SUPPORTED or
SMCCC_RET_INVALID_PARAMETER, it seems it will fall through to the default
case and return 0 (success).

Could this cause the driver to silently ignore hardware or firmware failures
and proceed with potentially corrupted state?

> +}
[ ... ]
> @@ -162,8 +223,18 @@ static int ufs_mtk_phy_probe(struct platform_device *pdev)
>  	if (IS_ERR(phy->mmio))
>  		return PTR_ERR(phy->mmio);
>  
> +	phy->reset = devm_reset_control_get_optional_exclusive(dev, NULL);
> +	if (IS_ERR(phy->reset))
> +		return dev_err_probe(dev, PTR_ERR(phy->reset), "Failed to get reset\n");
> +
>  	phy->dev = dev;
>  
> +	if (phy->reset) {
> +		ret = ufs_mtk_phy_ctrl(phy, UFS_MPHY_BACKUP);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "Failed to back up MPHY\n");

[Severity: High]
Does this create a strict probe failure on systems with older firmware?

If a Device Tree accurately describing the hardware includes the reset line,
but the system is running an older ATF firmware that lacks the
UFS_MPHY_BACKUP SMC call, ufs_mtk_phy_ctrl() returns -EOPNOTSUPP.

Could this cause the driver probe to fail entirely, rendering the PHY and
dependent UFS host non-functional? Might it be better to degrade
gracefully if the optional firmware backup command is unsupported?

> +	}
> +
>  	ret = ufs_mtk_phy_clk_init(phy);
>  	if (ret)
>  		return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001-mt8196-ufs-v13-0-60854adab6d9@collabora.com?part=5