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

[Yixun Lan <dlan@kernel.org>]

Hi ALL,

On 12:46 Mon 21 Sep     , sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] scsi: ufs: spacemit: missing M-PHY TX EQ configuration for lane 1
> - [Medium] scsi: ufs: spacemit: unchecked return value from quirk configuration
> --
> 
> commit 16f84e82c7f6101530a90dc4cc7509a53d6ac1b4
> Author: Yixun Lan <dlan@kernel.org>
> 
> scsi: ufs: spacemit: k3: Add UFS Host Controller driver
> 
> This commit introduces the UFS host controller driver for the SpacemiT K3
> SoC. It implements the vendor-specific configurations and initialization
> sequences required to interact with the M-PHY and UniPro hardware.
> 
> > diff --git a/drivers/ufs/host/ufs-spacemit.c b/drivers/ufs/host/ufs-spacemit.c
> > new file mode 100644
> > index 0000000000000..941506cc5ab68
> > --- /dev/null
> > +++ b/drivers/ufs/host/ufs-spacemit.c
> 
> [ ... ]
> 
> > @@ -211,6 +211,9 @@ static int ufs_spacemit_uniprov1p6_init(struct ufs_hba *hba)
> > +		/* RX HB8_TIME CAP */
> > +		{ UIC_ARG_MIB_SEL(0x0092, UIC_ARG_MPHY_RX_GEN_SEL_INDEX(0)), 0x64 },
> > +		{ UIC_ARG_MIB_SEL(0x0092, UIC_ARG_MPHY_RX_GEN_SEL_INDEX(1)), 0x64 },
> > +		/* TX EQ 3DB */
> > +		{ UIC_ARG_MIB_SEL(0x00CD, UIC_ARG_MPHY_TX_GEN_SEL_INDEX(0)),  0x5 },
> 
> [Severity: Medium]
> The TX EQ 3DB attribute is explicitly configured for lane 0, but the
> corresponding configuration for lane 1 (UIC_ARG_MPHY_TX_GEN_SEL_INDEX(1))
> is missing in ufs_spacemit_uniprov1p6_init(), unlike other dual-lane
> attributes in this array.
> 
> Can this missing equalization on lane 1 cause signal integrity issues and a
> high error rate on the second TX lane, potentially degrading performance or
> causing transmission failures unconditionally during the UniPro initialization
> sequence when the device operates in 2-lane mode?
ignore this comment, the configuration will also apply to lane 1

> 
> > +		/* RX garbage cnt = 32 SI */
> > +		{ UIC_ARG_MIB_SEL(0x00F2, UIC_ARG_MPHY_RX_GEN_SEL_INDEX(0)), 0x9F },
> > +		{ UIC_ARG_MIB_SEL(0x00F2, UIC_ARG_MPHY_RX_GEN_SEL_INDEX(1)), 0x9F },
> > +	};
> 
> [ ... ]
> 
> > @@ -401,6 +401,9 @@ static int ufs_spacemit_apply_dev_quirks(struct ufs_hba *hba)
> > +	int err;
> > +
> > +	if (hba->dev_quirks & UFS_DEVICE_QUIRK_HOST_PA_SAVECONFIGTIME)
> > +		ufs_spacemit_quirk_host_pa_saveconfigtime(hba);
> 
> [Severity: Medium]
> The return value of ufs_spacemit_quirk_host_pa_saveconfigtime() is neither
> assigned nor checked in ufs_spacemit_apply_dev_quirks(), ignoring potential
> DME access errors.
> 
I will add a check and abort if it fails

> If the PA_SaveConfigTime quirk configuration fails, could the error be
> silently ignored, causing the device to operate with an invalid timing
> configuration and leading to link instability or failure when a device
> requiring UFS_DEVICE_QUIRK_HOST_PA_SAVECONFIGTIME is initialized?
> 
> > +
> > +	if (hba->dev_info.wmanufacturerid == UFS_VENDOR_WDC)
> > +		hba->dev_quirks |= UFS_DEVICE_QUIRK_HOST_PA_TACTIVATE;
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260921-08-k3-ufs-support-v6-0-3479768a2299@kernel.org?part=2

-- 
Yixun Lan (dlan)