Re: [PATCH v10 06/26] scsi: ufs: mediatek: Rework resets

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Hardware Reset Sequence Violation due to inverted bulk reset array order and improper PHY synchronization.
- [High] Missing mandatory hardware reset delay when UFS MPHY is unbound.
--

--- Patch [6]: [PATCH v10 06/26] scsi: ufs: mediatek: Rework resets ---
commit 6e1eee5b92733c43e2ae7add5906da2c59fbe389
Author: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

scsi: ufs: mediatek: Rework resets

This commit reworks the reset control getting in the driver's probe
function to use the bulk reset APIs instead of getting resets individually.
It also removes all remnants of the MPHY reset from this driver.

> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
> --- a/drivers/ufs/host/ufs-mediatek.c
> +++ b/drivers/ufs/host/ufs-mediatek.c
> @@ -93,6 +93,12 @@ static const char *const ufs_uic_dl_err_str[] = {
>  	"PA_INIT"
>  };
>  
> +static const char *const ufs_reset_names[] = {
> +	"unipro",
> +	"crypto",
> +	"hci",
> +};

[Severity: High]
Does this array ordering accidentally reverse the original hardware reset
sequence?

The original code explicitly asserted resets in the order: HCI, Crypto,
UniPro, and deasserted them in the order: UniPro, Crypto, HCI.

Because reset_control_bulk_assert() iterates forwards and
reset_control_bulk_deassert() iterates backwards, this new array ordering
will assert UniPro, Crypto, HCI, and deassert HCI, Crypto, UniPro.

[ ... ]

> @@ -203,49 +209,45 @@ static void ufs_mtk_crypto_enable(struct ufs_hba *hba)
>  static void ufs_mtk_host_reset(struct ufs_hba *hba)
>  {
>  	struct ufs_mtk_host *host = ufshcd_get_variant(hba);
> -	struct arm_smccc_res res;
> -
> -	reset_control_assert(host->hci_reset);
> -	reset_control_assert(host->crypto_reset);
> -	reset_control_assert(host->unipro_reset);
> -	reset_control_assert(host->mphy_reset);
> -
> -	usleep_range(100, 110);
>  
> -	reset_control_deassert(host->unipro_reset);
> -	reset_control_deassert(host->crypto_reset);
> -	reset_control_deassert(host->hci_reset);
> -	reset_control_deassert(host->mphy_reset);
> +	int ret;
>  
> -	/* restore mphy setting aftre mphy reset */
> -	if (host->mphy_reset)
> -		ufs_mtk_mphy_ctrl(UFS_MPHY_RESTORE, res);
> -}
> +	ret = reset_control_bulk_assert(MTK_UFS_NUM_RESETS, host->resets);
> +	if (ret)
> +		dev_warn(hba->dev, "Host reset assert failed: %pe\n", ERR_PTR(ret));
>  
> +	ret = phy_reset(host->mphy);

[Severity: High]
Are we restoring the PHY while the MAC/host controller blocks are still held
in reset?

In the original code, the MPHY SMC restore was called after all controller
blocks were deasserted. Here, phy_reset() executes the full MPHY reset and
restore before reset_control_bulk_deassert() is called.

> +
> +	/*
> +	 * Only sleep if MPHY doesn't have a reset implemented (which already
> +	 * sleeps) or the PHY reset function failed somehow, just to be safe
> +	 */
> +	if (ret) {
> +		usleep_range(100, 110);
> +		if (ret != -EOPNOTSUPP)
> +			dev_warn(hba->dev, "PHY reset failed: %pe\n", ERR_PTR(ret));
>  	}

[Severity: High]
Does this skip the mandatory hardware reset delay when the UFS MPHY is
unbound?

If host->mphy is NULL, phy_reset(NULL) will return 0. This causes the if (ret)
condition to evaluate to false, bypassing the usleep_range(100, 110)
fallback.

This would result in the MAC/HCI resets being asserted and deasserted
instantaneously back-to-back, which can cause the controller IP block to
fail to reset.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730-mt8196-ufs-v10-0-bc5ecef202dc@collabora.com?part=6

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy