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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] scsi: ufs: spacemit: 100ms timeout on 1-lane UFS devices during hibern8
- [Medium] scsi: ufs: spacemit: bRefClkGatingWaitTime violation after hibern8 enter
- [Medium] scsi: ufs: spacemit: simultaneous ref clock enable and reset de-assertion
--

commit efcda2c96a818b5a53e91fa2558654c937a9ba68
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 support for features compatible with JEDEC UFS 2.2, MIPI
UniPro v1.61, and the 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
> --- /dev/null
> +++ b/drivers/ufs/host/ufs-spacemit.c

[ ... ]

> +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: Medium]
Will this loop always hit its 100ms timeout on 1-lane UFS devices?

It appears that on a 1-lane device, the state for lane 1 will not reach
TX_FSM_HIBERN8, causing the do-while condition to wait until retries run out
before proceeding.

[ ... ]

> +static int ufs_spacemit_device_reset(struct ufs_hba *hba)
> +{
> +	/* Disable reference clock & asserted ufs device reset */
> +	ufshcd_writel(hba, 0x000, UFS_PHY_MNG_BASE + UFS_DEVICE_IO_CTRL);
> +	usleep_range(10, 15);
> +
> +	/* Enable reference clock & deassert ufs device reset */
> +	ufshcd_writel(hba, MPHY_DEVICE_REFCLK_OE | MPHY_DEVICE_HW_RESET,
> +		      UFS_PHY_MNG_BASE + UFS_DEVICE_IO_CTRL);

[Severity: Medium]
Could toggling the reference clock output enable and device reset
de-assertion in a single MMIO write cause device initialization failures?

The UFS specification (e.g., JESD220C 5.3.1.2) typically requires the
reference clock to be valid and stable before RST_n is de-asserted. Writing
MPHY_DEVICE_REFCLK_OE and MPHY_DEVICE_HW_RESET simultaneously might violate
this sequential stability requirement.

[ ... ]

> +static void ufs_spacemit_post_hibern8(struct ufs_hba *hba, enum uic_cmd_dme cmd)
> +{

[ ... ]

> +	if (cmd == UIC_CMD_DME_HIBER_ENTER) {
> +		err = ufs_spacemit_check_hibern8(hba);
> +		if (err)
> +			dev_err(hba->dev, "ufs_spacemit_check_hibern8 error\n");
> +
> +		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);
> +
> +		/* Disable reference clock, keep device reset deasserted */
> +		ufshcd_writel(hba, MPHY_DEVICE_HW_RESET,
> +			      UFS_PHY_MNG_BASE + UFS_DEVICE_IO_CTRL);
> +	}
> +}

[Severity: Medium]
Does disabling the reference clock immediately after confirming HIBERN8
enter violate the device's clock gating wait time?

By not waiting for hba->dev_info.clk_gating_wait_us (bRefClkGatingWaitTime)
before disabling the clock, this could potentially cause strict UFS devices
to hang or fail internal state transitions during hibernation.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261002-08-k3-ufs-support-v7-0-bb39007ff9ae@kernel.org?part=2