Re: [PATCH v5 2/3] 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:
- [High] scsi: ufs: missing M-PHY attributes configuration for lane 1
- [Low] scsi: ufs: dead code via unused struct ufs_spacemit_host
- [Low] scsi: ufs: unused macros and magic numbers in M-PHY config
--

commit 82db4e922a94ed053811c2ea8440617bd6a94d08
Author: Yixun Lan <dlan@kernel.org>
Subject: scsi: ufs: spacemit: k3: Add UFS Host Controller driver

This commit adds the UFS Host Controller driver for the SpacemiT K3 SoC.
The hardware features are compatible with JEDEC UFS 2.2, MIPI UniPro v1.61, 
and M-PHY v3.0 standards.

> diff --git a/drivers/ufs/host/ufs-spacemit.c b/drivers/ufs/host/ufs-spacemit.c
> index 0000000000000..179e6edb2ac25
> --- /dev/null
> +++ b/drivers/ufs/host/ufs-spacemit.c

[ ... ]

> +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 */
> +		{ UIC_ARG_MIB(0x1553), 0xf }, /* PA_TXHSG1PREPARELENGTH */
> +		{ UIC_ARG_MIB(0x1554), 0x4f }, /* PA_TXHSG2SYNCLENGTH */
> +		{ UIC_ARG_MIB(0x1555), 0xf }, /* PA_TXHSG2PREPARELENGTH */

[Severity: Low]
Is there a reason the raw hex literals are used here instead of the
macros like PA_TXHSG1SYNCLENGTH and PA_TXHSG1PREPARELENGTH that were
explicitly defined at the top of the file?

[ ... ]

> +		/* TX HB8_TIME CAP */
> +		{ UIC_ARG_MIB_SEL(0x000F, UIC_ARG_MPHY_TX_GEN_SEL_INDEX(0)), 0x64 },
> +		{ UIC_ARG_MIB_SEL(0x000F, UIC_ARG_MPHY_TX_GEN_SEL_INDEX(1)), 0x64 },
> +		/* 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 },
> +		/* RX garbage cnt = 32 SI */

[Severity: High]
Should the TX EQ 3DB attribute also be configured for lane 1 here? The
surrounding attributes (like TX/RX HB8_TIME CAP and RX garbage cnt) properly
target both lanes 0 and 1. 

If lane 1 is left asymmetrically configured, could this lead to PHY link
instability, CRC errors, or failure to maintain high-speed gears in a 2-lane
setup?

> +		{ 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 },
> +	};
> +
> +	return ufs_spacemit_dme_set(hba, setup_attrs, ARRAY_SIZE(setup_attrs));
> +}

[ ... ]

> +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 },
> +		{ UIC_ARG_MIB(DL_AFC0REQTIMEOUTVAL), UFS_DL_AFC0REQTIMEOUTVAL_MAX },
> +	};

[Severity: High]
Does this workaround need to be applied to lane 1 as well? The inline comment
explicitly documents this as a "2Lane" workaround, but the configuration is
only applied to lane 0 via UIC_ARG_MPHY_TX_GEN_SEL_INDEX(0).

[ ... ]

> +static int ufs_spacemit_pwr_change_notify(struct ufs_hba *hba,
> +					  enum ufs_notify_change_status status,
> +					  struct ufs_pa_layer_attr *dev_req_params)
> +{
> +	struct ufs_spacemit_host *host = ufshcd_get_variant(hba);
> +	int ret = 0;
> +
> +	if (!dev_req_params) {
> +		dev_err(hba->dev, "Invalid Parameters\n");
> +		return -EINVAL;
> +	}
> +
> +	switch (status) {
> +	case PRE_CHANGE:
> +		break;
> +	case POST_CHANGE:
> +		/* Cache the power mode parameters to use internally */
> +		memcpy(&host->dev_req_params, dev_req_params, sizeof(*dev_req_params));

[Severity: Low]
Are the cached power mode parameters used elsewhere in the driver? We are
copying dev_req_params into host->dev_req_params here, but the structure
field doesn't appear to be read anywhere else.

[ ... ]

> +static int ufs_spacemit_init(struct ufs_hba *hba)
> +{
> +	struct device *dev = hba->dev;
> +	struct ufs_spacemit_host *host;
> +	struct reset_control *rst;
> +
> +	host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
> +	if (!host)
> +		return -ENOMEM;

[Severity: Low]
Since the ufs_spacemit_host structure is only written to and never read, do
we need to allocate this memory during initialization, or is there some
planned functionality missing that was supposed to consume it?

> +
> +	rst = devm_reset_control_get_exclusive_deasserted(dev, NULL);

[ ... ]

> diff --git a/drivers/ufs/host/ufs-spacemit.h b/drivers/ufs/host/ufs-spacemit.h
> index 0000000000000..5e459f6ba243c
> --- /dev/null
> +++ b/drivers/ufs/host/ufs-spacemit.h

[ ... ]

> +/* ATOP base*/
> +#define UFS_ATOP_BASE			0x1C00
> +
> +#define UFS_SYSCLK			499

[Severity: Low]
Is this UFS_SYSCLK macro intended to be used? It doesn't appear to be
referenced anywhere in the implementation.

> +#define UFS_TX_SYMBO_CLK		0x800
> +#define UFS_MAX_LINKSTARTUP_TIMER	0xFFFFFFFF
> +#define UFS_DL_AFC0REQTIMEOUTVAL_MAX	0xFFFF

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916-08-k3-ufs-support-v5-0-1f5d71f1027d@kernel.org?part=2