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

[Yixun Lan <dlan@kernel.org>]

Hi Yao,

On 17:21 Thu 02 Jul     , Yao Zi wrote:
> On Thu, Jul 02, 2026 at 02:31:36AM +0000, Yixun Lan wrote:
> > 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.
> > 
> > Signed-off-by: Yixun Lan <dlan@kernel.org>
> > ---
> >  drivers/ufs/host/Kconfig        |  12 +
> >  drivers/ufs/host/Makefile       |   1 +
> >  drivers/ufs/host/ufs-spacemit.c | 931 ++++++++++++++++++++++++++++++++++++++++
> >  drivers/ufs/host/ufs-spacemit.h |  90 ++++
> >  4 files changed, 1034 insertions(+)
> 
> ...
> 
> > +static int ufs_spacemit_wait_mphy_pll_lock(struct ufs_hba *hba)
> > +{
> > +	int timeout = MPHY_PLL_LOCK_TIMEOUT_US;
> > +	u32 val;
> > +
> > +	while (timeout-- > 0) {
> > +		val = ufshcd_readl(hba, UFS_PHY_MNG_BASE + UFS_MPHY_PU_CTRL);
> > +		if (val & MPHY_PLL_LOCK_BIT)
> > +			return 0;
> > +
> > +		udelay(1);
> > +	}
> > +
> > +	dev_err(hba->dev, "M-PHY PLL lock timeout\n");
> > +	return -ETIMEDOUT;
> > +}
> 
> Could this loop be replaced by read_poll_timeout() like
> 
> 	read_poll_timeout(ufshcd_readl, val, val & MPHY_PLL_LOCK_BIT,
> 			 1, MPHY_PLL_LOCK_TIMEOUT_US, false, hba,
> 			 UFS_PHY_MSG_BASE + UFS_MPHY_PU_CTRL);
> 
I think so, thanks for the suggestion

> 
> ...
> 
> > +/**
> > + * ufs_spacemit_init - init phy and prepare clk
> > + * @hba: host controller instance
> > + */
> > +static int ufs_spacemit_init(struct ufs_hba *hba)
> > +{
> > +	int err = 0;
> > +	struct device *dev = hba->dev;
> > +	struct ufs_spacemit_host *host;
> > +
> > +	host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
> > +	if (!host)
> > +		return -ENOMEM;
> > +
> > +	host->rst = devm_reset_control_get_optional_exclusive_deasserted(dev, NULL);
> 
> "resets" property is marked as required in the binding, but the optional
> API is used here. Is this expected?
> 
Ok, I will switch to non-optional API and check return value

> ...
> 
> > +/**
> > + * ufs_spacemit_hce_enable_notify - Configure HCE enable sequence
> > + * @hba: host controller instance
> > + * @status: notification status (PRE_CHANGE or POST_CHANGE)
> > + *
> > + * Configures host controller enable with proper sequencing.
> > + * Handles crypto enable if supported.
> > + *
> > + * Returns: 0 on success
> > + */
> > +static int ufs_spacemit_hce_enable_notify(struct ufs_hba *hba,
> > +					  enum ufs_notify_change_status status)
> > +{
> > +	struct ufs_spacemit_host *host = ufshcd_get_variant(hba);
> > +	u32 enable_val, val;
> > +
> > +	if (status == PRE_CHANGE) {
> > +		enable_val = CONTROLLER_ENABLE;
> > +
> > +		if (hba->caps & UFSHCD_CAP_CRYPTO)
> > +			enable_val = CRYPTO_GENERAL_ENABLE | CONTROLLER_ENABLE;
> > +
> > +		if (!host->first_hce_done) {
> > +			host->first_hce_done = true;
> > +			dev_dbg(hba->dev, "First HCE enable\n");
> > +		} else {
> > +			val = ufshcd_readl(hba, REG_CONTROLLER_ENABLE);
> > +			if (val == enable_val) {
> > +				ufshcd_writel(hba, enable_val & (1 << CONTROLLER_ENABLE),
> > +					      REG_CONTROLLER_ENABLE);
> > +
> > +				while (ufshcd_readl(hba, REG_CONTROLLER_ENABLE) ==
> > +				       (enable_val & (1 << CONTROLLER_ENABLE)))
> > +					;
> 
> Shouldn't we set a timeout for the polling loop?
> 
Yes, good idea and will do in next version

-- 
Yixun Lan (dlan)