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

[Yixun Lan <dlan@kernel.org>]

Hi All,

On 07:55 Tue 08 Sep     , sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Missing lane 1 configuration for M-PHY tuning attribute 0xe8.
> - [High] The SpacemiT UFS platform driver fails to configure PM (Power Management) operations, breaking system suspend/resume.
> --
> 
> commit e432cf6faf83c7fe0612ce4cb8a3f5d0731598d9
> Author: Yixun Lan <dlan@kernel.org>
> 
> 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..9d037d90ac5e3
> > --- /dev/null
> > +++ b/drivers/ufs/host/ufs-spacemit.c
> [ ... ]
> > +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]
> Is Lane 1 tuning intentionally omitted here?
> 
> The driver declares support for 2 TX lanes, and the comment mentions
> configuring "2Lane@409M". However, the M-PHY tuning attribute 0xe8 is
> only configured for Lane 0 via UIC_ARG_MPHY_TX_GEN_SEL_INDEX(0).
> 
> Operating in 2-lane mode without applying the same PHY tuning to the
> second lane can cause link instability or negotiation failures. Should
> this also configure Lane 1?
> 
No need, this is false report, the configuration will apply to Lane 1

or anyone has idea to silence this when sending next version?

> [ ... ]
> > +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),
> > +	},
> > +};
> > +module_platform_driver(ufs_spacemit_pltform);
> 
> [Severity: High]
> Does this driver correctly configure power management?
> 
> The platform driver structure does not initialize the pm field in its
> driver sub-structure. Without a valid dev_pm_ops structure, the platform
> bus will not invoke any PM callbacks for this device.
> 
> This means the UFS host controller will not participate in system
> suspend/resume or runtime power management, which can lead to power

The K3 mainline platform lacks low level support for system suspend/resume,
while for runtime power management, I think it's a valid point, and
should be feasible to implement, so will take a look in next version.

> drain or system crashes on resume if the power domain was gated by
> firmware. Should this assign standard UFS PM callbacks to the pm field?
> 

Thanks

-- 
Yixun Lan (dlan)