Re: [PATCH v6 2/2] scsi: ufs: spacemit: k3: Add UFS Host Controller driver

["Troy Mitchell" <troy.mitchell@linux.spacemit.com>]

[-- Attachment #1: Type: text/plain, Size: 1347 bytes --]

On Mon Sep 21, 2026 at 8:36 PM +08, Yixun Lan wrote:
> [...]
>
> +static int ufs_spacemit_link_startup_pre_change(struct ufs_hba *hba)
> +{
> +	u32 value, sys1clk_1us;
> +	int err;
> +
> +	err = ufs_spacemit_mphy_init(hba);
> +	if (err < 0)
> +		return err;
> +
> +	err = ufs_spacemit_uniprov1p6_init(hba);
> +	if (err < 0)
> +		return err;
> +

Should this be `if (err)`? The call chain is:

  ufs_spacemit_uniprov1p6_init()
    -> ufs_spacemit_dme_set()
      -> ufshcd_dme_set()
        -> ufshcd_dme_set_attr()

ufshcd_dme_set_attr() returns 0 on success and non-zero on failure,
including positive UIC error codes. The current check misses those.

Also, ufshcd_link_startup() ignores the PRE_CHANGE return value in this
series' base, so returning an error here alone will not abort startup.

> [...]
>
> +static int ufs_spacemit_apply_dev_quirks(struct ufs_hba *hba)
> +{
> [...]
> +	err = ufs_spacemit_dme_set(hba, setup_attrs, ARRAY_SIZE(setup_attrs));
> +	if (err < 0)
> +		return err;
The same `if (err)` check is needed here for positive DME errors.

> +
> +	return ufs_spacemit_wait_mphy_pll_lock(hba);
> +}
> +
> [...]
>
> +static int ufs_spacemit_init(struct ufs_hba *hba)
> +{
> [...]
> +	hba->caps |= UFSHCD_CAP_RPM_AUTOSUSPEND;
> +
> +	return 0;
> +}
> +

-- 
Troy Mitchell

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 248 bytes --]