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

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Vendor M-PHY TX attributes are only configured for TX Lane 0, neglecting Lane 1 in a 2-lane setup.
- [High] Hardcoded dual-lane check in `ufs_spacemit_check_hibern8` will fail and timeout on 1-lane UFS devices.
- [High] The UFS hardware reset pulse width is too short, violating the JEDEC UFS specification.
- [Medium] Misleading macro names and comments for device reset contradict hardware behavior.
- [Medium] Silently ignoring DME configuration errors.
- [Low] Usage of `of_match_ptr` causes build warnings when `CONFIG_OF` is disabled.
--

--- Patch [2]: [PATCH v3 2/3] scsi: ufs: spacemit: k3: Add UFS Host Controller driver ---
commit a6bb8f511d306cddb9d21e41123fafdbcd63a6c9
Author: Yixun Lan <[email protected]>
Subject: scsi: ufs: spacemit: k3: Add UFS Host Controller driver

SpacemiT K3 SoC consist of UFS (Universal Flash Storage) Host Controller
which has features compatible with JEDEC UFS 2.2, MIPI UniPro v1.61 and
M-PHY v3.0 standard.

> diff --git a/drivers/ufs/host/ufs-spacemit.c b/drivers/ufs/host/ufs-spacemit.c
> new file mode 100644
> index 0000000000000..94942ea27fb7d
> --- /dev/null
> +++ b/drivers/ufs/host/ufs-spacemit.c

[ ... ]

> +/* PHY register magic values */
> +#define MPHY_PU_ALL			0x87f
> +#define MPHY_PU_WITH_HB8_RESET		0xb7f
> +#define MPHY_DEVICE_RESET_DEASSERT	0x101
> +#define MPHY_DEVICE_RESET_ASSERT	0x001

[Severity: Medium]
Is this macro name misleading? Looking at ufs_spacemit_device_reset(), it
uses 0x000 to assert the reset and 0x101 to de-assert it. This means 0x001
actually disables the reference clock while keeping the reset de-asserted.

[ ... ]

> +static int ufs_spacemit_check_hibern8(struct ufs_hba *hba)
> +{
> +	u32 tx_fsm_val_0 = 0;
> +	u32 tx_fsm_val_1 = 0;
> +	int retries = DIV_ROUND_UP(HBRN8_POLL_TOUT_MS * 1000, 100);
> +	int err = 0;
> +
> +	do {
> +		err = ufshcd_dme_get(hba,
> +				     UIC_ARG_MIB_SEL(MPHY_TX_FSM_STATE,
> +						     UIC_ARG_MPHY_TX_GEN_SEL_INDEX(0)),
> +				     &tx_fsm_val_0);
> +		if (err)
> +			break;
> +
> +		err = ufshcd_dme_get(hba,
> +				     UIC_ARG_MIB_SEL(MPHY_TX_FSM_STATE,
> +						     UIC_ARG_MPHY_TX_GEN_SEL_INDEX(1)),
> +				     &tx_fsm_val_1);
> +		if (err || (tx_fsm_val_0 == TX_FSM_HIBERN8 &&
> +			    tx_fsm_val_1 == TX_FSM_HIBERN8))
> +			break;

[Severity: High]
Will this hardcoded dual-lane check timeout on single-lane UFS devices?

If a 1-lane memory chip is connected, Lane 1 is inactive. Querying its state
will either return a DME error or read a non-Hibern8 state, causing the loop
to timeout after 100ms and break power management.

Could this check be updated to use the dynamically negotiated lane count from
dev_req_params to know how many lanes to poll?

[ ... ]

> +static int ufs_spacemit_uniprov1p6_init(struct ufs_hba *hba)
> +{
> +	static const struct ufshcd_dme_attr_val setup_attrs[] = {
> +		{ UIC_ARG_MIB(0x1552), 0x4f }, /* PA_TXHSG1SYNCLENGTH */
[ ... ]
> +		/* 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: High]
Should this TX equalization be applied to Lane 1 as well?

The host controller is configured for 2 lanes, but this setup attribute only
targets UIC_ARG_MPHY_TX_GEN_SEL_INDEX(0). Omitting tuning parameters for the
second lane could cause asymmetric lane configurations or link instability.

[ ... ]

> +static int ufs_spacemit_link_startup_post_change(struct ufs_hba *hba)
> +{
> +	/* Add 0xe8 make UFS2.1 run GEAR3 + 2Lane@409M */
> +	static const struct ufshcd_dme_attr_val setup_attrs[] = {
> +		{ UIC_ARG_MIB_SEL(0xe8, UIC_ARG_MPHY_TX_GEN_SEL_INDEX(0)), 0x97 },
> +		{ UIC_ARG_MIB_SEL(0xe8, UIC_ARG_MPHY_TX_GEN_SEL_INDEX(0)), 0xd7 },
> +		{ UIC_ARG_MIB_SEL(0xe8, UIC_ARG_MPHY_TX_GEN_SEL_INDEX(0)), 0x17 },

[Severity: High]
Does attribute 0xe8 need to be programmed for both lanes?

Similar to the TX equalization setup, this only programs Lane 0 despite the
driver specifying dual-lane support.

[ ... ]

> +static int ufs_spacemit_apply_dev_quirks(struct ufs_hba *hba)
> +{
> +	static const struct ufshcd_dme_attr_val setup_attrs[] = {
[ ... ]
> +	};
> +
> +	if (hba->dev_quirks & UFS_DEVICE_QUIRK_HOST_PA_SAVECONFIGTIME)
> +		ufs_spacemit_quirk_host_pa_saveconfigtime(hba);
> +
> +	if (hba->dev_info.wmanufacturerid == UFS_VENDOR_WDC)
> +		hba->dev_quirks |= UFS_DEVICE_QUIRK_HOST_PA_TACTIVATE;
> +
> +	ufs_spacemit_dme_set(hba, setup_attrs, ARRAY_SIZE(setup_attrs));

[Severity: Medium]
Is it safe to ignore the return values of these DME configuration functions?

If ufs_spacemit_quirk_host_pa_saveconfigtime() or ufs_spacemit_dme_set()
fail, the errors are not propagated. Could this allow initialization to proceed
with an incorrectly configured M-PHY?

[ ... ]

> +static int ufs_spacemit_device_reset(struct ufs_hba *hba)
> +{
> +	/* Stop device ref_clk & asserted ufs device reset */
> +	ufshcd_writel(hba, 0x000, UFS_PHY_MNG_BASE + UFS_DEVICE_IO_CTRL);
> +	usleep_range(10, 15);

[Severity: High]
Is this reset pulse width long enough?

The JEDEC UFS specification mandates that the host must maintain the RST_n
signal low for a minimum of 1 ms. A 10 to 15 microsecond delay might cause
the memory chip to fail to reset properly.

[ ... ]

> +static void ufs_spacemit_post_hibern8(struct ufs_hba *hba, enum uic_cmd_dme cmd)
> +{
> +	static const struct ufshcd_dme_attr_val setup_attrs[] = {
> +		{ UIC_ARG_MIB_SEL(0xf1, UIC_ARG_MPHY_RX_GEN_SEL_INDEX(0)), 0x84 },
> +		{ UIC_ARG_MIB_SEL(0xf1, UIC_ARG_MPHY_RX_GEN_SEL_INDEX(1)), 0x84 },
> +		{ UIC_ARG_MIB_SEL(0xf1, UIC_ARG_MPHY_RX_GEN_SEL_INDEX(0)), 0x80 },
> +		{ UIC_ARG_MIB_SEL(0xf1, UIC_ARG_MPHY_RX_GEN_SEL_INDEX(1)), 0x80 },
> +		{ UIC_ARG_MIB(0xdd), 0x57 },
> +		{ UIC_ARG_MIB(0xdd), 0xd7 },
> +		{ UIC_ARG_MIB(0xe8), 0x57 },
> +		{ UIC_ARG_MIB(0xe8), 0xd7 },

[Severity: High]
Are these attributes defaulting to Lane 0?

Using UIC_ARG_MIB() defaults to Lane 0. If these need to be applied to Lane 1
as well during post-Hibern8 configuration, they are currently missing.

[ ... ]

> +	if (cmd == UIC_CMD_DME_HIBER_ENTER) {
> +		ufs_spacemit_check_hibern8(hba);
> +
> +		ufs_spacemit_dme_set(hba, setup_attrs, ARRAY_SIZE(setup_attrs));
> +
> +		/* Power down M-PHY */
> +		ufshcd_writel(hba, 0x0, UFS_PHY_MNG_BASE + UFS_MPHY_PU_CTRL);
> +
> +		/* Keep reference clock enabled, assert device reset */
> +		ufshcd_writel(hba, MPHY_DEVICE_RESET_ASSERT,
> +			      UFS_PHY_MNG_BASE + UFS_DEVICE_IO_CTRL);

[Severity: Medium]
Does this operation contradict its comment?

The code writes MPHY_DEVICE_RESET_ASSERT (0x001), which disables the reference
clock but keeps the reset de-asserted. The comment says "Keep reference clock
enabled, assert device reset", which is the exact opposite.

While keeping reset de-asserted is the correct action for Hibern8 (asserting
reset would destroy the device's state), the comment and macro name suggest
incorrect hardware behavior.

[ ... ]

> +static struct platform_driver ufs_spacemit_pltform = {
> +	.probe	= ufs_spacemit_probe,
> +	.remove	= ufs_spacemit_remove,
> +	.driver	= {
> +		.name	= "ufshcd-spacemit",
> +		.of_match_table = of_match_ptr(ufs_spacemit_of_match),
> +	},
> +};

[Severity: Low]
Does the use of of_match_ptr() here cause a compiler warning?

Because ufs_spacemit_of_match is defined unconditionally, building with
CONFIG_OF disabled will evaluate of_match_ptr() to NULL, leaving the array
unused and triggering a -Wunused-variable warning.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2