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

[Yixun Lan <dlan@kernel.org>]

Hi ALL,

On 03:10 Wed 16 Sep     , sashiko-bot@kernel.org wrote:
> 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?
> 
I found there are some macros defined in incude/ufs/unipro.h already
I will use them while leave others unchanged which not defined in global

I will also drop local macros from this drivers for now, it's probably
better to promote them into global header, but I'd prefer to do it in
a separate patch

> [ ... ]
> 
> > +		/* 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?
> 
false report, the configuration will actually apply to lane 1 too, so ignore

> > +		{ 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).
> 
ditto, wil ignore 

> [ ... ]
> 
> > +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.
> 
it's true, I will drop this cache parameters ..

> [ ... ]
> 
> > +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?
>
I think this due to massive cleanup from vendor driver, but I would keep
it as is for now, for easy to implement vendor specific feature.

> > +
> > +	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.
> 
will drop it

> > +#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

-- 
Yixun Lan (dlan)